Web Repository - Code Quality Report¶

Executive Summary¶

This report analyzes code quality, technical debt, and adherence to best practices in the Psyter Web Application. The analysis identifies areas requiring refactoring, maintainability improvements, and code standardization.

Assessment Date: November 2025
Priority Levels: 🔴 Critical | 🟠 High | 🟡 Medium | 🟢 Low

Overall Code Quality Score: 5.5/10

Code Quality Issues¶

1. Controller Size & Complexity (🔴 Critical)¶

Issue: Several controllers exceed 1,000+ lines, violating Single Responsibility Principle

Affected Files:

ServiceProviderController.cs    3,700+ lines  🔴
UserManagerController.cs         2,800+ lines  🔴
ClientController.cs             2,500+ lines  🔴
PaymentController.cs            1,800+ lines  🟠
CommonController.cs             1,500+ lines  🟠
CatalogueController.cs          1,200+ lines  🟠
PhysicianAvailabilityController 1,100+ lines  🟠

Problems:
- Difficult to maintain and test
- Mixed responsibilities
- High cognitive load
- Merge conflicts
- Violation of SOLID principles

Recommendations (Do Now):
1. Split by feature areas:

// Instead of one large ServiceProviderController:
ServiceProviderProfileController
ServiceProviderAppointmentController
ServiceProviderScheduleController
ServiceProviderFinanceController

Extract to service layer:

public class AppointmentService
{
    public async Task<BookingResult> CreateBooking(BookingRequest request)
    {
        // Business logic here
    }
}

public class AppointmentController : Controller
{
    private readonly AppointmentService _appointmentService;

    public async Task<ActionResult> Book(BookingViewModel model)
    {
        var result = await _appointmentService.CreateBooking(model.ToRequest());
        return View(result);
    }
}

Use areas for logical grouping
Maximum 500 lines per controller

Impact: High - improves maintainability significantly

2. Lack of Service Layer (🔴 Critical)¶

Issue: Business logic directly in controllers, no separation of concerns

Current Pattern:

public async Task<ActionResult> GetProfile()
{
    // Direct API calls in controller
    var dalManager = new DALManager(ApiType.psyter);
    JObject response = await dalManager.GetData(...);
    var profileResponse = response.ToObject<UserProfileResponse>();

    // Business logic in controller
    if (profileResponse.Status == 1)
    {
        // Process data
        // Transform data
        // Validation
    }

    return View(profileResponse);
}

Problems:
- Cannot unit test business logic
- Code duplication across controllers
- Difficult to change business rules
- Tight coupling to API layer

Recommendations (Do Now):

// Create service interfaces
public interface IUserService
{
    Task<UserProfile> GetProfileAsync(int userId);
    Task<bool> UpdateProfileAsync(UserProfile profile);
}

// Implement services
public class UserService : IUserService
{
    private readonly IApiDataAccess _apiDataAccess;

    public UserService(IApiDataAccess apiDataAccess)
    {
        _apiDataAccess = apiDataAccess;
    }

    public async Task<UserProfile> GetProfileAsync(int userId)
    {
        // Business logic here
        var response = await _apiDataAccess.GetUserProfile(userId);
        return MapToModel(response);
    }
}

// Use in controller
public class UserController : Controller
{
    private readonly IUserService _userService;

    public UserController(IUserService userService)
    {
        _userService = userService;
    }

    public async Task<ActionResult> Profile()
    {
        var profile = await _userService.GetProfileAsync(CurrentUserId);
        return View(profile);
    }
}

Impact: High - enables testing, reduces coupling

3. Missing Dependency Injection (🟠 High)¶

Issue: Manual object creation, tight coupling, difficult to test

Current Pattern:

// Tight coupling, cannot mock
var dalManager = new DALManager(ApiType.psyter);
var apiDataAccess = new ApiDataAccess();

Problems:
- Cannot inject mock dependencies for testing
- Hard-coded dependencies
- Difficult to swap implementations
- Violation of Dependency Inversion Principle

Recommendations (Do Next):

// 1. Install DI container (built-in)
// 2. Register services
public static void RegisterServices(IServiceCollection services)
{
    // Register services
    services.AddScoped<IUserService, UserService>();
    services.AddScoped<IApiDataAccess, ApiDataAccess>();
    services.AddScoped<IDALManager, DALManager>();

    // Register HTTP clients
    services.AddHttpClient<IApiDataAccess, ApiDataAccess>();
}

// 3. Use constructor injection
public class UserController : Controller
{
    private readonly IUserService _userService;
    private readonly ILogger<UserController> _logger;

    public UserController(IUserService userService, ILogger<UserController> logger)
    {
        _userService = userService;
        _logger = logger;
    }
}

Impact: High - enables testing, improves flexibility

4. Code Duplication (🟠 High)¶

Issue: Repeated code patterns across controllers and views

Examples:

Language Selection Logic (repeated in every controller):

// Duplicated in multiple controllers
string viewPath = "";
if (Session[SessionVariables.CurrentLanguage].ToString() == AppConstants.Languages.English)
{
    viewPath = "~/Views/Web/English/Home.cshtml";
}
else
{
    viewPath = "~/Views/Web/Arabic/Home.cshtml";
}
return View(viewPath);

Solution:

// Create base controller
public abstract class LocalizedController : Controller
{
    protected string GetLocalizedView(string viewName)
    {
        var language = Session[SessionVariables.CurrentLanguage]?.ToString() 
                       ?? AppConstants.Languages.English;
        var folder = language == AppConstants.Languages.English ? "English" : "Arabic";
        return $"~/Views/{ControllerContext.RouteData.Values["controller"]}/{folder}/{viewName}.cshtml";
    }

    protected new ViewResult View(string viewName, object model = null)
    {
        return base.View(GetLocalizedView(viewName), model);
    }
}

// Use in controllers
public class WebController : LocalizedController
{
    public async Task<ActionResult> Home()
    {
        return View("Home", pageDetails); // Automatically selects language
    }
}

Session Data Retrieval:

// Duplicated everywhere
var userInfo = (UserLoginInfo)Session[SessionVariables.UserLoginInfo];
var apiTokens = (APIAuthTokensResponse)Session[SessionVariables.APIAuthTokenList];

Solution:

public abstract class BaseController : Controller
{
    protected UserLoginInfo CurrentUser => 
        Session[SessionVariables.UserLoginInfo] as UserLoginInfo;

    protected APIAuthTokensResponse ApiTokens => 
        Session[SessionVariables.APIAuthTokenList] as APIAuthTokensResponse;

    protected int CurrentUserId => CurrentUser?.UserID ?? 0;
    protected string CurrentLanguage => 
        Session[SessionVariables.CurrentLanguage]?.ToString() 
        ?? AppConstants.Languages.English;
}

Impact: Medium - reduces code, improves maintainability

5. Poor Error Handling (🟠 High)¶

Issue: Inconsistent error handling, generic catch blocks, poor error messages

Current Patterns:

// Problem: Generic catch-all
try
{
    var result = await dalManager.GetData(...);
    return View(result);
}
catch (Exception ex)
{
    // Generic error handling
    return View("Error");
}

// Problem: No logging
catch (Exception ex)
{
    // Exception swallowed
}

// Problem: No user feedback
catch
{
    return RedirectToAction("Error");
}

Recommendations (Do Now):

// Create custom exceptions
public class ApiException : Exception
{
    public int StatusCode { get; set; }
    public string ErrorCode { get; set; }

    public ApiException(string message, int statusCode, string errorCode) 
        : base(message)
    {
        StatusCode = statusCode;
        ErrorCode = errorCode;
    }
}

// Implement proper error handling
public async Task<ActionResult> GetProfile()
{
    try
    {
        var profile = await _userService.GetProfileAsync(CurrentUserId);
        return View(profile);
    }
    catch (ApiException ex)
    {
        _logger.LogError(ex, "API error getting profile for user {UserId}", CurrentUserId);
        TempData["ErrorMessage"] = "Unable to load profile. Please try again.";
        return RedirectToAction("Dashboard");
    }
    catch (Exception ex)
    {
        _logger.LogCritical(ex, "Unexpected error getting profile for user {UserId}", CurrentUserId);
        return View("Error", new ErrorViewModel { Message = "An unexpected error occurred." });
    }
}

// Global exception filter
public class GlobalExceptionFilter : IExceptionFilter
{
    private readonly ILogger<GlobalExceptionFilter> _logger;

    public void OnException(ExceptionContext context)
    {
        _logger.LogError(context.Exception, "Unhandled exception");

        context.Result = new ViewResult
        {
            ViewName = "Error",
            ViewData = new ViewDataDictionary(new ErrorViewModel
            {
                RequestId = Activity.Current?.Id ?? context.HttpContext.TraceIdentifier
            })
        };

        context.ExceptionHandled = true;
    }
}

Impact: Medium - better diagnostics, user experience

6. Hardcoded Strings & Magic Numbers (🟡 Medium)¶

Issue: Hardcoded values throughout code

Examples:

// Magic numbers
if (userInfo.RoleID == 1)  // What is 1?
if (status == 3)           // What is 3?

// Hardcoded strings
if (language == "en")
var path = "~/Views/Web/English/Home.cshtml"

// Hardcoded URLs
var url = "https://api.psyter.com/user/profile"

Recommendations (Do Next):

// Use enums
public enum UserRole
{
    Admin = 1,
    Client = 2,
    ServiceProvider = 3,
    Accountant = 4
}

public enum AppointmentStatus
{
    Pending = 1,
    Confirmed = 2,
    Completed = 3,
    Cancelled = 4
}

// Use constants
public static class Languages
{
    public const string English = "en";
    public const string Arabic = "ar";
}

// Use configuration
private readonly string _apiBaseUrl = ConfigurationManager.AppSettings["PsyterApiBasePath"];

Impact: Low-Medium - improves readability

7. Inconsistent Naming Conventions (🟡 Medium)¶

Issues:
- Inconsistent controller method names
- Variable naming inconsistency
- Abbreviations without consistency

Examples:

// Inconsistent prefixes
var lStrViewPath       // Hungarian notation
var pageDetails        // Camel case
string viewPath        // Normal

Recommendations (Do Next):
1. Follow Microsoft C# naming conventions
2. Use PascalCase for public members
3. Use camelCase for private fields
4. Avoid abbreviations
5. Use meaningful names

Impact: Low - improves readability

8. Lack of Unit Tests (🔴 Critical)¶

Issue: No visible unit tests for web application

Problems:
- Cannot verify correctness
- Regression risks
- Difficult to refactor safely
- No test-driven development

Recommendations (Do Now):

// Create test project
Psyter.Web.Tests

// Install testing frameworks
- xUnit or NUnit
- Moq (mocking framework)
- FluentAssertions

// Example tests
public class UserServiceTests
{
    private readonly Mock<IApiDataAccess> _mockApi;
    private readonly UserService _userService;

    public UserServiceTests()
    {
        _mockApi = new Mock<IApiDataAccess>();
        _userService = new UserService(_mockApi.Object);
    }

    [Fact]
    public async Task GetProfile_ValidUser_ReturnsProfile()
    {
        // Arrange
        var userId = 123;
        var expectedProfile = new UserProfile { UserId = userId };
        _mockApi.Setup(x => x.GetUserProfile(userId))
                .ReturnsAsync(expectedProfile);

        // Act
        var result = await _userService.GetProfileAsync(userId);

        // Assert
        result.Should().NotBeNull();
        result.UserId.Should().Be(userId);
    }
}

Test Coverage Goals:
- Business logic: 80%+
- Controllers: 60%+
- Critical paths: 100%

Impact: High - prevents regressions, enables refactoring

9. View Duplication (English/Arabic) (🟠 High)¶

Issue: Complete duplication of views for each language

Problems:
- Maintenance nightmare
- Inconsistencies between languages
- Double the code to update
- Bugs in one language version

Current Structure:

Views/
├── Web/
│   ├── English/
│   │   ├── Home.cshtml
│   │   ├── About.cshtml
│   │   └── ...
│   └── Arabic/
│       ├── Home.cshtml
│       ├── About.cshtml
│       └── ...

Recommendations (Do Next):

// Use resource files
Resources/
├── Views.Home.resx (English)
└── Views.Home.ar.resx (Arabic)

// Single view with localization
@using Microsoft.AspNetCore.Mvc.Localization
@inject IViewLocalizer Localizer

<h1>@Localizer["WelcomeTitle"]</h1>
<p>@Localizer["WelcomeMessage"]</p>

// Or use shared views with culture-specific partials
Views/
├── Web/
│   ├── Home.cshtml (shared)
│   └── Partials/
│       ├── _Header.en.cshtml
│       └── _Header.ar.cshtml

// Apply RTL dynamically
<html dir="@(CultureInfo.CurrentCulture.Name == "ar" ? "rtl" : "ltr")">

Impact: High - reduces maintenance burden

10. Session Overuse (🟠 High)¶

Issue: Heavy reliance on session state, scalability concern

Current Usage:

Session[SessionVariables.UserLoginInfo]
Session[SessionVariables.APIAuthTokenList]
Session[SessionVariables.UserProfileInfo]
Session[SessionVariables.CurrentLanguage]
Session[SessionVariables.SelectedCareProvider]
Session[SessionVariables.BookingInfo]
// ... many more

Problems:
- Scalability issues (server affinity required)
- Memory consumption
- Load balancing difficulties
- Session timeout issues

Recommendations (Plan):

// 1. Use claims-based authentication
var claims = new List<Claim>
{
    new Claim(ClaimTypes.NameIdentifier, user.Id.ToString()),
    new Claim(ClaimTypes.Email, user.Email),
    new Claim(ClaimTypes.Role, user.Role),
    new Claim("Language", user.PreferredLanguage)
};

// 2. Use distributed cache for temporary data
public class CachedBookingService
{
    private readonly IDistributedCache _cache;

    public async Task SaveBookingDraft(BookingDraft draft)
    {
        var key = $"booking:{draft.UserId}:{draft.Id}";
        await _cache.SetStringAsync(key, JsonSerializer.Serialize(draft), 
            new DistributedCacheEntryOptions 
            { 
                AbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(30) 
            });
    }
}

// 3. Use cookies for non-sensitive preferences
Response.Cookies.Append("Language", language, new CookieOptions
{
    Expires = DateTimeOffset.UtcNow.AddYears(1),
    HttpOnly = true,
    Secure = true
});

Impact: High - improves scalability

11. Missing Input Validation (🟠 High)¶

Issue: Inconsistent server-side validation

Current State:
- Some models have data annotations
- Not all inputs validated
- Client-side validation only in places

Recommendations (Do Now):

// Use data annotations
public class UserRegistrationViewModel
{
    [Required(ErrorMessage = "Email is required")]
    [EmailAddress(ErrorMessage = "Invalid email format")]
    public string Email { get; set; }

    [Required]
    [StringLength(100, MinimumLength = 8, ErrorMessage = "Password must be 8-100 characters")]
    [RegularExpression(@"^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]", 
        ErrorMessage = "Password must contain uppercase, lowercase, number, and special character")]
    public string Password { get; set; }

    [Required]
    [Compare("Password", ErrorMessage = "Passwords do not match")]
    public string ConfirmPassword { get; set; }

    [Required]
    [Phone(ErrorMessage = "Invalid phone number")]
    public string PhoneNumber { get; set; }
}

// Validate in controller
[HttpPost]
[ValidateAntiForgeryToken]
public async Task<ActionResult> Register(UserRegistrationViewModel model)
{
    if (!ModelState.IsValid)
    {
        return View(model);
    }

    // Additional business validation
    if (await _userService.EmailExistsAsync(model.Email))
    {
        ModelState.AddModelError("Email", "Email already in use");
        return View(model);
    }

    // Proceed with registration
}

// Create custom validators
public class ValidSaudiIdAttribute : ValidationAttribute
{
    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        var id = value as string;
        if (string.IsNullOrWhiteSpace(id) || id.Length != 10 || !id.All(char.IsDigit))
        {
            return new ValidationResult("Invalid Saudi ID format");
        }
        return ValidationResult.Success;
    }
}

Impact: Medium - prevents bad data

12. Async/Await Misuse (🟡 Medium)¶

Issue: Inconsistent async patterns, potential deadlocks

Problems:

// Problem: Blocking on async code
var result = SomeAsyncMethod().Result;  // Can deadlock

// Problem: Not awaiting
public async Task<ActionResult> Method()
{
    var result = SomeAsyncMethod();  // Not awaited
    return View();
}

// Problem: Async void
public async void ProcessData()  // Should be async Task
{
    // ...
}

Recommendations (Do Next):

// Correct patterns
public async Task<ActionResult> Method()
{
    var result = await SomeAsyncMethod();
    return View(result);
}

// Configure await when context not needed
public async Task<Data> GetDataAsync()
{
    var result = await _api.GetAsync().ConfigureAwait(false);
    return result;
}

// Use async all the way
public async Task<ActionResult> Index()
{
    var data = await _service.GetDataAsync();
    return View(data);
}

Impact: Medium - prevents deadlocks

Code Metrics Analysis¶

Cyclomatic Complexity¶

High Complexity Methods (>15):
- ServiceProviderController.SaveProfile() - ~25
- ClientController.BookAppointment() - ~22
- PaymentController.ProcessPayment() - ~20
- UserManagerController.Register() - ~18

Recommendations:
- Break down complex methods
- Extract helper methods
- Use strategy pattern for conditional logic

Code Coverage¶

Current: 0% (no tests)
Target: 70%+

Priority Areas:
1. Business logic (services) - 80%+
2. Controllers - 60%+
3. Utilities - 90%+
4. Models - 30%+ (validation logic)

Technical Debt¶

Estimated Technical Debt: 180-250 hours

Breakdown:
1. Controller refactoring: 60-80 hours
2. Service layer creation: 80-120 hours
3. Unit test creation: 60-80 hours
4. View consolidation: 40-60 hours
5. DI implementation: 20-30 hours
6. Error handling: 10-15 hours
7. Code cleanup: 20-30 hours

Best Practices Violations¶

SOLID Principles¶

Single Responsibility: ❌
- Controllers do too much
- Mixed business logic and presentation

Open/Closed: ⚠️
- Hard to extend without modification

Liskov Substitution: ✅
- Inheritance used appropriately

Interface Segregation: ❌
- Few interfaces defined

Dependency Inversion: ❌
- Direct dependencies on concrete classes

DRY (Don’t Repeat Yourself)¶

Violations: Many
- View duplication
- Session access code
- API call patterns
- Language selection

KISS (Keep It Simple)¶

Violations: Some
- Overly complex controllers
- Nested conditionals
- Long methods

Priority Action Plan¶

Do Now (0-30 days) 🔴¶

Start unit testing framework (10 hrs)
Create base controller for common logic (5 hrs)
Implement proper error handling (10 hrs)
Extract 1-2 services from largest controllers (20 hrs)
Add input validation to critical endpoints (15 hrs)
Fix async/await patterns (8 hrs)

Total Effort: ~68 hours

Do Next (30-90 days) 🟠¶

Split large controllers (40 hrs)
Implement dependency injection (25 hrs)
Create service layer (80 hrs)
Consolidate views with localization (50 hrs)
Add comprehensive unit tests (60 hrs)
Standardize naming conventions (10 hrs)

Total Effort: ~265 hours

Plan (90+ days) 🟡¶

Reduce session usage (50 hrs)
Implement CQRS pattern (100 hrs)
Add integration tests (40 hrs)
Code quality automation (SonarQube) (20 hrs)
Performance profiling (30 hrs)
Modernize to .NET 6/8 (200+ hrs)

Total Effort: ~440+ hours

Code Quality Tools¶

Recommended Tools¶

Static Analysis:
- SonarQube/SonarCloud
- ReSharper/Rider
- StyleCop
- FxCop
Testing:
- xUnit/NUnit
- Moq
- FluentAssertions
- Bogus (test data)
Code Coverage:
- Coverlet
- ReportGenerator
- Azure DevOps Code Coverage
Performance:
- MiniProfiler
- Application Insights
- dotTrace

Metrics to Track¶

Monthly:
- Lines of code
- Cyclomatic complexity
- Code coverage %
- Technical debt hours
- Code smells (SonarQube)
- Duplicated code %
- Maintainability index

Per Release:
- Refactored classes
- New unit tests
- Test coverage delta
- Resolved code smells

Conclusion¶

The Psyter Web Application has significant code quality issues, primarily around:
1. Large, complex controllers requiring refactoring
2. No service layer or dependency injection
3. Lack of unit tests (0% coverage)
4. View duplication for localization
5. Inconsistent error handling

Overall Assessment: Requires substantial refactoring to meet industry standards.

Recommended Approach:
1. Stop adding features temporarily
2. Focus on core refactoring (controllers, services)
3. Implement testing framework
4. Gradual improvement over 3-6 months

Document Version: 1.0
Last Updated: November 2025
Next Review: Monthly during refactoring period

Keys	Action
`?`	Open this help
`n`	Next page
`p`	Previous page
`s`	Search