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&#x3C;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:

First occurrence - just write the code
Second occurrence - note it, consider extraction
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&#x3C;T>(T obj) =>
    System.Text.Json.JsonSerializer.Serialize(obj, Options);

public static T? Deserialize&#x3C;T>(string json) =>
    System.Text.Json.JsonSerializer.Deserialize&#x3C;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&#x3C;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&#x3C;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&#x3C;Product?> GetByIdAsync(int id) =>
    _repository.GetByIdAsync(id);

}

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

The Cost of YAGNI Violations

Development time: Building unused features
Maintenance burden: More code to maintain
Complexity: Harder to understand system
Testing overhead: More tests for unused code
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.

Safety Notice

Copy this and send it to your AI assistant to learn

Source Transparency

Related Skills

codebase-analysis

development-workflow

requirements-clarification

brainstorming