Clean Code

Clean Code Principles

DRY - Don't Repeat Yourself

Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

Code Duplication

// BAD: Duplicated validation logic
public class UserService
{
    public void CreateUser(string email, string name)
    {
        if (string.IsNullOrWhiteSpace(email) || !email.Contains("@"))
            throw new ArgumentException("Invalid email");
        // ...
    }

    public void UpdateEmail(int userId, string newEmail)
    {
        if (string.IsNullOrWhiteSpace(newEmail) || !newEmail.Contains("@"))
            throw new ArgumentException("Invalid email");
        // ...
    }
}

// GOOD: Single source of truth
public class UserService
{
    private readonly IEmailValidator _emailValidator;

    public void CreateUser(string email, string name)
    {
        _emailValidator.ValidateOrThrow(email);
        // ...
    }

    public void UpdateEmail(int userId, string newEmail)
    {
        _emailValidator.ValidateOrThrow(newEmail);
        // ...
    }
}

public class EmailValidator : IEmailValidator
{
    public bool IsValid(string email) =>
        !string.IsNullOrWhiteSpace(email) && email.Contains("@");

    public void ValidateOrThrow(string email)
    {
        if (!IsValid(email))
            throw new ArgumentException("Invalid email", nameof(email));
    }
}

Magic Numbers and Strings

// BAD: Magic values scattered throughout code
public decimal CalculateDiscount(decimal total)
{
    if (total > 100)
        return total * 0.1m;  // What is 100? What is 0.1?
    return 0;
}

public bool IsEligibleForFreeShipping(decimal total)
{
    return total > 100;  // Duplicated magic number!
}

// GOOD: Named constants
public static class OrderThresholds
{
    public const decimal FreeShippingMinimum = 100m;
    public const decimal StandardDiscountRate = 0.10m;
}

public decimal CalculateDiscount(decimal total)
{
    if (total > OrderThresholds.FreeShippingMinimum)
        return total * OrderThresholds.StandardDiscountRate;
    return 0;
}

public bool IsEligibleForFreeShipping(decimal total)
{
    return total > OrderThresholds.FreeShippingMinimum;
}

Configuration Duplication

// BAD: Connection strings in multiple places
public class OrderRepository
{
    private readonly string _conn = "Server=prod;Database=Orders;";
}

public class CustomerRepository
{
    private readonly string _conn = "Server=prod;Database=Orders;";
}

// GOOD: Centralized configuration
public class DatabaseOptions
{
    public string ConnectionString { get; set; } = string.Empty;
}

// In startup
services.Configure<DatabaseOptions>(configuration.GetSection("Database"));

// In repositories
public class OrderRepository
{
    private readonly string _connectionString;

    public OrderRepository(IOptions<DatabaseOptions> options)
    {
        _connectionString = options.Value.ConnectionString;
    }
}

When DRY Goes Wrong (WET is Sometimes Better)

// Over-DRY: Forced abstraction hurts readability
public T ProcessEntity<T>(T entity, Func<T, bool> validator, Action<T> processor)
    where T : class
{
    if (!validator(entity))
        throw new ValidationException();
    processor(entity);
    return entity;
}

// Better: Some duplication is acceptable for clarity
public Order ProcessOrder(Order order)
{
    ValidateOrder(order);
    SaveOrder(order);
    return order;
}

public Customer ProcessCustomer(Customer customer)
{
    ValidateCustomer(customer);
    SaveCustomer(customer);
    return customer;
}

Rule of Three

Only extract duplication after you've seen it THREE times: 1. First occurrence - just write the code 2. Second occurrence - note it, consider extraction 3. Third occurrence - refactor to remove duplication

---

KISS - Keep It Simple, Stupid

The simplest solution is usually the best solution.

Over-Engineering

// BAD: Over-engineered for simple use case
public interface IUserNameFormatter
{
    string Format(User user);
}

public abstract class UserNameFormatterBase : IUserNameFormatter
{
    protected abstract string GetFirstNamePart(User user);
    protected abstract string GetLastNamePart(User user);

    public string Format(User user) =>
        $"{GetFirstNamePart(user)} {GetLastNamePart(user)}";
}

public class StandardUserNameFormatter : UserNameFormatterBase
{
    protected override string GetFirstNamePart(User user) => user.FirstName;
    protected override string GetLastNamePart(User user) => user.LastName;
}

public class UserNameFormatterFactory
{
    public IUserNameFormatter Create(string type) => type switch
    {
        "standard" => new StandardUserNameFormatter(),
        _ => throw new NotSupportedException()
    };
}

// GOOD: Simple and direct
public static class UserExtensions
{
    public static string GetFullName(this User user) =>
        $"{user.FirstName} {user.LastName}";
}

Premature Abstraction

// BAD: Abstraction for one implementation
public interface IOrderIdGenerator
{
    string Generate();
}

public class GuidOrderIdGenerator : IOrderIdGenerator
{
    public string Generate() => Guid.NewGuid().ToString();
}

// Registration
services.AddSingleton<IOrderIdGenerator, GuidOrderIdGenerator>();

// GOOD: Direct until you need flexibility
public class Order
{
    public string Id { get; } = Guid.NewGuid().ToString();
}

// Add abstraction ONLY when you need a second implementation

Complex LINQ vs Simple Loops

// BAD: Hard to understand nested LINQ
var result = orders
    .Where(o => o.Status == OrderStatus.Completed)
    .GroupBy(o => o.CustomerId)
    .Select(g => new
    {
        CustomerId = g.Key,
        TotalSpent = g.Sum(o => o.Total),
        OrderCount = g.Count(),
        AverageOrder = g.Average(o => o.Total)
    })
    .Where(x => x.TotalSpent > 1000)
    .OrderByDescending(x => x.TotalSpent)
    .Take(10)
    .SelectMany(x => customers.Where(c => c.Id == x.CustomerId)
        .Select(c => new CustomerReport
        {
            Name = c.Name,
            Email = c.Email,
            TotalSpent = x.TotalSpent,
            OrderCount = x.OrderCount
        }))
    .ToList();

// GOOD: Break into readable steps
var completedOrders = orders.Where(o => o.Status == OrderStatus.Completed);

var customerOrderSummaries = completedOrders
    .GroupBy(o => o.CustomerId)
    .Select(g => new CustomerOrderSummary(
        CustomerId: g.Key,
        TotalSpent: g.Sum(o => o.Total),
        OrderCount: g.Count()))
    .Where(s => s.TotalSpent > 1000)
    .OrderByDescending(s => s.TotalSpent)
    .Take(10)
    .ToList();

var customerLookup = customers.ToDictionary(c => c.Id);

var reports = customerOrderSummaries
    .Select(s => CreateReport(s, customerLookup[s.CustomerId]))
    .ToList();

Boolean Parameters

// BAD: What does 'true' mean?
SendEmail(user, "Welcome!", true, false, true);

// GOOD: Named parameters or dedicated methods
SendEmail(user, "Welcome!",
    isHtml: true,
    includeAttachments: false,
    trackOpens: true);

// Even better: Specific methods
SendWelcomeEmail(user);
SendPasswordResetEmail(user);

Excessive Inheritance

// BAD: Deep inheritance hierarchy
public abstract class Entity { }
public abstract class AuditableEntity : Entity { }
public abstract class SoftDeletableAuditableEntity : AuditableEntity { }
public class Order : SoftDeletableAuditableEntity { }

// GOOD: Composition and interfaces
public interface IAuditable
{
    DateTime CreatedAt { get; }
    DateTime? ModifiedAt { get; }
}

public interface ISoftDeletable
{
    bool IsDeleted { get; }
    DateTime? DeletedAt { get; }
}

public class Order : IAuditable, ISoftDeletable
{
    public DateTime CreatedAt { get; init; }
    public DateTime? ModifiedAt { get; private set; }
    public bool IsDeleted { get; private set; }
    public DateTime? DeletedAt { get; private set; }
}

---

YAGNI - You Aren't Gonna Need It

Don't implement something until it is necessary.

Feature Creep

// BAD: Building for hypothetical future requirements
public class UserService
{
    public User CreateUser(
        string email,
        string name,
        string? middleName = null,           // No requirement for this
        string? suffix = null,                // No requirement for this
        string? preferredName = null,         // No requirement for this
        bool enableTwoFactor = false,         // No requirement for this
        string? backupEmail = null,           // No requirement for this
        Dictionary<string, string>? metadata = null)  // "Might need it later"
    {
        // ...
    }
}

// GOOD: Only what's needed now
public class UserService
{
    public User CreateUser(string email, string name)
    {
        return new User
        {
            Id = Guid.NewGuid(),
            Email = email,
            Name = name,
            CreatedAt = DateTime.UtcNow
        };
    }
}

Unnecessary Flexibility

// BAD: Configurable everything (but we only use JSON)
public interface ISerializer
{
    string Serialize<T>(T obj);
    T Deserialize<T>(string data);
}

public class JsonSerializer : ISerializer { }
public class XmlSerializer : ISerializer { }
public class YamlSerializer : ISerializer { }
public class BinarySerializer : ISerializer { }
public class MessagePackSerializer : ISerializer { }

public class SerializerFactory
{
    public ISerializer Create(string format) => // ...
}

// GOOD: Use what you need
public static class JsonHelper
{
    private static readonly JsonSerializerOptions Options = new()
    {
        PropertyNamingPolicy = JsonNamingPolicy.CamelCase
    };

    public static string Serialize<T>(T obj) =>
        System.Text.Json.JsonSerializer.Serialize(obj, Options);

    public static T? Deserialize<T>(string json) =>
        System.Text.Json.JsonSerializer.Deserialize<T>(json, Options);
}

Unused Abstractions

// BAD: Interface with single implementation, no plans for others
public interface IEmailSender
{
    Task SendAsync(string to, string subject, string body);
}

public class SmtpEmailSender : IEmailSender
{
    public Task SendAsync(string to, string subject, string body)
    {
        // Only implementation we'll ever have
    }
}

// GOOD: Just use the class directly
public class EmailSender
{
    public async Task SendAsync(string to, string subject, string body)
    {
        // ...
    }
}

// Add interface WHEN you actually need a second implementation

Premature Optimization

// BAD: Caching before measuring
public class ProductService
{
    private readonly IMemoryCache _cache;
    private readonly IDistributedCache _distributedCache;
    private readonly IProductRepository _repository;

    public async Task<Product?> GetByIdAsync(int id)
    {
        var cacheKey = $"product_{id}";

        // Check L1 cache
        if (_cache.TryGetValue(cacheKey, out Product? product))
            return product;

        // Check L2 cache
        var cached = await _distributedCache.GetStringAsync(cacheKey);
        if (cached != null)
        {
            product = JsonSerializer.Deserialize<Product>(cached);
            _cache.Set(cacheKey, product, TimeSpan.FromMinutes(5));
            return product;
        }

        // Database fallback
        product = await _repository.GetByIdAsync(id);
        if (product != null)
        {
            var serialized = JsonSerializer.Serialize(product);
            await _distributedCache.SetStringAsync(cacheKey, serialized);
            _cache.Set(cacheKey, product, TimeSpan.FromMinutes(5));
        }

        return product;
    }
}

// GOOD: Start simple, optimize when needed
public class ProductService
{
    private readonly IProductRepository _repository;

    public Task<Product?> GetByIdAsync(int id) =>
        _repository.GetByIdAsync(id);
}

// Add caching AFTER you've identified it as a bottleneck

The Cost of YAGNI Violations

1. Development time: Building unused features 2. Maintenance burden: More code to maintain 3. Complexity: Harder to understand system 4. Testing overhead: More tests for unused code 5. Technical debt: May become outdated or incompatible

---

Clean Code Checklist

Naming

• [ ] Names reveal intent
• [ ] Names are searchable
• [ ] No encoded type information (Hungarian notation)
• [ ] Consistent naming conventions

Functions

• [ ] Small (< 20 lines preferred)
• [ ] Do one thing
• [ ] One level of abstraction
• [ ] Few parameters (< 3 preferred)
• [ ] No side effects
• [ ] Command/Query separation

Comments

• [ ] Code is self-documenting
• [ ] Comments explain WHY, not WHAT
• [ ] No commented-out code
• [ ] XML docs for public APIs

Formatting

• [ ] Consistent indentation
• [ ] Logical grouping of related code
• [ ] Blank lines separate concepts
• [ ] Team style guide followed

Error Handling

• [ ] Exceptions, not error codes
• [ ] Specific exception types
• [ ] No empty catch blocks
• [ ] Fail fast principle

See reference.md for more examples.

Clean Code Reference

Naming Conventions

Classes and Methods

// BAD
public class Mgr { }
public void DoIt() { }
public int Calc(int x) { }

// GOOD
public class OrderManager { }
public void ProcessPayment() { }
public int CalculateTotalDiscount(int orderId) { }

Boolean Names

// BAD
bool flag;
bool open;
bool status;

// GOOD
bool isActive;
bool hasPermission;
bool canExecute;
bool shouldRetry;

Collections

// BAD
List<User> list;
Dictionary<int, Order> dict;

// GOOD
List<User> activeUsers;
Dictionary<int, Order> ordersByCustomerId;

Avoid Mental Mapping

// BAD
for (int i = 0; i < users.Length; i++)
    for (int j = 0; j < users[i].Orders.Length; j++)
        Process(users[i].Orders[j]);

// GOOD
foreach (var user in users)
    foreach (var order in user.Orders)
        Process(order);

Function Design

Single Level of Abstraction

// BAD: Mixed abstraction levels
public void ProcessOrder(Order order)
{
    // High level
    ValidateOrder(order);

    // Low level - detail leak
    using var conn = new SqlConnection(_connectionString);
    conn.Open();
    var cmd = new SqlCommand("INSERT INTO Orders...", conn);
    cmd.ExecuteNonQuery();

    // High level again
    SendConfirmationEmail(order);
}

// GOOD: Consistent abstraction
public async Task ProcessOrderAsync(Order order)
{
    await ValidateOrderAsync(order);
    await SaveOrderAsync(order);
    await NotifyCustomerAsync(order);
}

Prefer Exceptions Over Error Codes

// BAD
public int CreateUser(User user)
{
    if (user == null) return -1;
    if (string.IsNullOrEmpty(user.Email)) return -2;
    if (UserExists(user.Email)) return -3;
    // ...
    return 0; // Success
}

// GOOD
public void CreateUser(User user)
{
    ArgumentNullException.ThrowIfNull(user);

    if (string.IsNullOrEmpty(user.Email))
        throw new ValidationException("Email is required");

    if (UserExists(user.Email))
        throw new DuplicateUserException(user.Email);

    // ...
}

Guard Clauses

// BAD: Deep nesting
public decimal CalculateDiscount(Order order)
{
    if (order != null)
    {
        if (order.Customer != null)
        {
            if (order.Customer.IsPremium)
            {
                if (order.Total > 100)
                {
                    return order.Total * 0.2m;
                }
            }
        }
    }
    return 0;
}

// GOOD: Guard clauses
public decimal CalculateDiscount(Order order)
{
    if (order?.Customer is not { IsPremium: true })
        return 0;

    if (order.Total <= 100)
        return 0;

    return order.Total * 0.2m;
}

Error Handling

Specific Exceptions

// BAD
catch (Exception ex)
{
    Log(ex);
    throw;
}

// GOOD
catch (SqlException ex) when (ex.Number == 2627)
{
    throw new DuplicateEntityException("Entity already exists", ex);
}
catch (SqlException ex) when (ex.Number == 547)
{
    throw new ForeignKeyViolationException("Related entity not found", ex);
}
catch (SqlException ex)
{
    _logger.LogError(ex, "Database error occurred");
    throw new DataAccessException("Database operation failed", ex);
}

Don't Return Null for Collections

// BAD
public List<Order>? GetOrdersByCustomer(int customerId)
{
    var orders = _repository.Find(customerId);
    return orders.Any() ? orders : null;
}

// GOOD
public IReadOnlyList<Order> GetOrdersByCustomer(int customerId)
{
    return _repository.Find(customerId);  // Returns empty list if none
}

Result Pattern for Expected Failures

// For operations that can fail in expected ways
public record Result<T>
{
    public bool IsSuccess { get; init; }
    public T? Value { get; init; }
    public string? Error { get; init; }

    public static Result<T> Success(T value) =>
        new() { IsSuccess = true, Value = value };

    public static Result<T> Failure(string error) =>
        new() { IsSuccess = false, Error = error };
}

public Result<User> AuthenticateUser(string email, string password)
{
    var user = _userRepository.GetByEmail(email);
    if (user == null)
        return Result<User>.Failure("Invalid credentials");

    if (!VerifyPassword(password, user.PasswordHash))
        return Result<User>.Failure("Invalid credentials");

    return Result<User>.Success(user);
}

Immutability

Prefer Immutable Objects

// BAD: Mutable
public class OrderItem
{
    public int ProductId { get; set; }
    public int Quantity { get; set; }
    public decimal Price { get; set; }
}

// GOOD: Immutable
public record OrderItem(int ProductId, int Quantity, decimal Price)
{
    public decimal Total => Quantity * Price;

    public OrderItem WithQuantity(int newQuantity) =>
        this with { Quantity = newQuantity };
}

Collection Encapsulation

// BAD: Exposing mutable collection
public class Order
{
    public List<OrderItem> Items { get; set; } = new();
}

// External code can do:
order.Items.Clear();
order.Items = null;

// GOOD: Encapsulated collection
public class Order
{
    private readonly List<OrderItem> _items = new();

    public IReadOnlyList<OrderItem> Items => _items.AsReadOnly();

    public void AddItem(OrderItem item)
    {
        ArgumentNullException.ThrowIfNull(item);
        _items.Add(item);
    }

    public void RemoveItem(int productId)
    {
        _items.RemoveAll(i => i.ProductId == productId);
    }
}

Dependency Injection Patterns

Constructor Injection (Preferred)

public class OrderService
{
    private readonly IOrderRepository _repository;
    private readonly ILogger<OrderService> _logger;

    public OrderService(IOrderRepository repository, ILogger<OrderService> logger)
    {
        _repository = repository ?? throw new ArgumentNullException(nameof(repository));
        _logger = logger ?? throw new ArgumentNullException(nameof(logger));
    }
}

Avoid Service Locator

// BAD: Service locator anti-pattern
public class OrderService
{
    public void Process()
    {
        var repo = ServiceLocator.Get<IOrderRepository>();
        var logger = ServiceLocator.Get<ILogger>();
    }
}

// GOOD: Explicit dependencies
public class OrderService
{
    private readonly IOrderRepository _repository;

    public OrderService(IOrderRepository repository)
    {
        _repository = repository;
    }
}

Async/Await Best Practices

ConfigureAwait

// Library code - use ConfigureAwait(false)
public async Task<Order> GetOrderAsync(int id)
{
    var data = await _httpClient.GetAsync($"/orders/{id}").ConfigureAwait(false);
    return await data.Content.ReadFromJsonAsync<Order>().ConfigureAwait(false);
}

// Application code - ConfigureAwait not needed (context flows automatically)
public async Task<IActionResult> GetOrder(int id)
{
    var order = await _orderService.GetOrderAsync(id);
    return Ok(order);
}

Avoid Async Void

// BAD: Exceptions are lost
public async void SendNotification()
{
    await _emailService.SendAsync(...);
}

// GOOD: Return Task
public async Task SendNotificationAsync()
{
    await _emailService.SendAsync(...);
}

// Exception: Event handlers can use async void
private async void OnButtonClick(object sender, EventArgs e)
{
    try
    {
        await ProcessAsync();
    }
    catch (Exception ex)
    {
        HandleError(ex);
    }
}

Don't Block on Async

// BAD: Deadlock risk
public void Process()
{
    var result = GetDataAsync().Result;  // Blocks!
    var result2 = GetDataAsync().GetAwaiter().GetResult();  // Also blocks!
}

// GOOD: Async all the way
public async Task ProcessAsync()
{
    var result = await GetDataAsync();
}

Code Organization

Vertical Ordering

public class OrderService
{
    // 1. Fields
    private readonly IOrderRepository _repository;
    private readonly ILogger _logger;

    // 2. Constructor
    public OrderService(IOrderRepository repository, ILogger logger)
    {
        _repository = repository;
        _logger = logger;
    }

    // 3. Public methods (high-level first)
    public async Task<Order> CreateOrderAsync(CreateOrderRequest request)
    {
        ValidateRequest(request);
        var order = BuildOrder(request);
        await SaveOrderAsync(order);
        return order;
    }

    // 4. Private methods (called in order they're used)
    private void ValidateRequest(CreateOrderRequest request) { }

    private Order BuildOrder(CreateOrderRequest request) { }

    private async Task SaveOrderAsync(Order order) { }
}

Newspaper Metaphor

• Headline (class name)
• Synopsis (public interface)
• Details (private implementation)
• Supporting details (utility methods)