Csharp Code Review

C#/.NET Code Review

Comprehensive code review checklist and patterns for C# and .NET applications, focusing on async patterns, LINQ, DI, EF Core, and modern C# features.

When to Use This Skill

• Reviewing C# and .NET pull requests
• Establishing .NET code review standards
• Training reviewers on C#-specific issues
• Catching async/await bugs and EF Core issues
• Ensuring proper dependency injection patterns

Quick Checklist

## C#/.NET Review Checklist

- [ ] async/await all the way (no .Result or .Wait())
- [ ] IDisposable properly disposed (using statement)
- [ ] Null checks with null-conditional operators
- [ ] LINQ optimized (no multiple enumeration)
- [ ] EF Core: AsNoTracking for read-only, Include for eager loading
- [ ] DI lifetime scopes correct (Scoped vs Singleton)
- [ ] ConfigureAwait(false) in library code
- [ ] Proper exception handling with filters

Review Severity Labels

🔴 [blocking]  - Must fix before merge (bugs, security, breaking)
🟡 [important] - Should fix, but discuss if you disagree
🟢 [nit]       - Nice to have, not blocking
💡 [suggestion]- Alternative approach to consider

Async/Await Issues

Blocking on Async - Deadlock Risk

// ❌ Blocking on async - DEADLOCK RISK in ASP.NET!
public User GetUser(int id)
{
    return GetUserAsync(id).Result;  // Deadlock!
}

public User GetUserAlt(int id)
{
    return GetUserAsync(id).GetAwaiter().GetResult();  // Still bad!
}

// ✅ Async all the way
public async Task<User> GetUserAsync(int id)
{
    return await _repository.GetByIdAsync(id);
}

async void - Unobserved Exceptions

// ❌ async void - Exceptions cannot be caught!
public async void SaveData(Data data)
{
    await _repository.SaveAsync(data);  // If this throws, app may crash!
}

// ❌ Also problematic in event handlers
button.Click += async void (sender, e) =>
{
    await DoWorkAsync();  // Exception crashes app!
};

// ✅ async Task for async methods
public async Task SaveDataAsync(Data data)
{
    await _repository.SaveAsync(data);
}

// ✅ For event handlers, wrap in try-catch
button.Click += async (sender, e) =>
{
    try
    {
        await DoWorkAsync();
    }
    catch (Exception ex)
    {
        Logger.Error(ex);
    }
};

Missing ConfigureAwait in Libraries

// ❌ Missing ConfigureAwait in library code
// Can cause deadlocks when called from UI thread
public async Task<Data> FetchDataAsync()
{
    var response = await _httpClient.GetAsync(url);  // Captures context
    return await response.Content.ReadFromJsonAsync<Data>();
}

// ✅ ConfigureAwait(false) in library code
public async Task<Data> FetchDataAsync()
{
    var response = await _httpClient.GetAsync(url).ConfigureAwait(false);
    return await response.Content.ReadFromJsonAsync<Data>().ConfigureAwait(false);
}

Sequential When Parallel Possible

// ❌ Sequential when parallel is possible
public async Task<UserData> GetUserDataAsync(int userId)
{
    var profile = await GetProfileAsync(userId);
    var posts = await GetPostsAsync(userId);      // Waits for profile!
    var followers = await GetFollowersAsync(userId);  // Waits for posts!

    return new UserData(profile, posts, followers);
}

// ✅ Parallel execution with Task.WhenAll
public async Task<UserData> GetUserDataAsync(int userId)
{
    var profileTask = GetProfileAsync(userId);
    var postsTask = GetPostsAsync(userId);
    var followersTask = GetFollowersAsync(userId);

    await Task.WhenAll(profileTask, postsTask, followersTask);

    return new UserData(
        profileTask.Result,  // Safe - task completed
        postsTask.Result,
        followersTask.Result
    );
}

Using ValueTask Incorrectly

// ❌ Awaiting ValueTask multiple times
public async Task ProcessAsync()
{
    ValueTask<int> valueTask = GetValueAsync();

    var result1 = await valueTask;
    var result2 = await valueTask;  // WRONG! Can't await twice!
}

// ✅ Await once or convert to Task
public async Task ProcessAsync()
{
    // Option 1: Await once
    var result = await GetValueAsync();

    // Option 2: Convert to Task if multiple awaits needed
    var task = GetValueAsync().AsTask();
    var result1 = await task;
    var result2 = await task;  // OK now
}

LINQ Issues

Multiple Enumeration

// ❌ Multiple enumeration - performance problem
public void ProcessUsers(IEnumerable<User> users)
{
    if (!users.Any()) return;           // First enumeration

    var count = users.Count();          // Second enumeration

    foreach (var user in users) { }     // Third enumeration!
}

// ✅ Materialize once
public void ProcessUsers(IEnumerable<User> users)
{
    var userList = users.ToList();  // Materialize once

    if (userList.Count == 0) return;

    foreach (var user in userList) { }
}

N+1 Query Problem

// ❌ N+1 queries - one query per order!
var orders = await _context.Orders.ToListAsync();
foreach (var order in orders)
{
    var customer = await _context.Customers
        .FirstAsync(c => c.Id == order.CustomerId);  // Query per order!
    order.CustomerName = customer.Name;
}

// ✅ Eager loading with Include
var orders = await _context.Orders
    .Include(o => o.Customer)  // Single query with JOIN
    .ToListAsync();

foreach (var order in orders)
{
    order.CustomerName = order.Customer.Name;  // Already loaded!
}

Loading Too Much Data

// ❌ Loading entire entity for few fields
var users = await _context.Users
    .Where(u => u.Active)
    .ToListAsync();  // Loads ALL columns

var names = users.Select(u => u.Name).ToList();

// ✅ Project to only needed fields
var names = await _context.Users
    .Where(u => u.Active)
    .Select(u => u.Name)  // Only fetch Name column
    .ToListAsync();

Deferred Execution Surprise

// ❌ Deferred execution in returned IEnumerable
public IEnumerable<User> GetActiveUsers()
{
    using var context = new AppDbContext();
    return context.Users.Where(u => u.Active);  // Not materialized!
}  // Context disposed before enumeration!

// ✅ Materialize before returning
public IEnumerable<User> GetActiveUsers()
{
    using var context = new AppDbContext();
    return context.Users.Where(u => u.Active).ToList();  // Materialized
}

// ✅ Or use async pattern
public async Task<List<User>> GetActiveUsersAsync()
{
    await using var context = new AppDbContext();
    return await context.Users.Where(u => u.Active).ToListAsync();
}

Client vs Server Evaluation

// ❌ Forces client evaluation - loads all data then filters
var users = await _context.Users
    .Where(u => MyCustomMethod(u.Name))  // Can't translate to SQL!
    .ToListAsync();

// ✅ Use SQL-translatable expressions
var users = await _context.Users
    .Where(u => u.Name.StartsWith("A"))  // Translates to SQL LIKE
    .ToListAsync();

// If custom logic needed, filter on server first
var users = await _context.Users
    .Where(u => u.Active)  // Server-side filter
    .ToListAsync();

var filtered = users.Where(u => MyCustomMethod(u.Name));  // Client-side

Dependency Injection Issues

Captive Dependency

// ❌ Captive dependency - Scoped injected into Singleton
services.AddSingleton<MySingleton>();
services.AddScoped<MyScopedService>();

public class MySingleton
{
    private readonly MyScopedService _scoped;  // WRONG! Scoped in Singleton!

    public MySingleton(MyScopedService scoped)
    {
        _scoped = scoped;  // This instance lives forever, never refreshed!
    }
}

// ✅ Use IServiceScopeFactory for scoped in singletons
public class MySingleton
{
    private readonly IServiceScopeFactory _scopeFactory;

    public MySingleton(IServiceScopeFactory scopeFactory)
    {
        _scopeFactory = scopeFactory;
    }

    public void DoWork()
    {
        using var scope = _scopeFactory.CreateScope();
        var scoped = scope.ServiceProvider.GetRequiredService<MyScopedService>();
        scoped.Process();
    }
}

Concrete Dependencies

// ❌ Depending on concrete implementation
public class OrderService
{
    private readonly SqlOrderRepository _repository;  // Concrete type!

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

// ✅ Depend on abstraction
public class OrderService
{
    private readonly IOrderRepository _repository;

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

Service Locator Anti-Pattern

// ❌ Service locator - hides dependencies
public class OrderProcessor
{
    public void Process(Order order)
    {
        var logger = ServiceLocator.Get<ILogger>();  // Hidden dependency!
        var repository = ServiceLocator.Get<IOrderRepository>();
        // ...
    }
}

// ✅ Constructor injection - explicit dependencies
public class OrderProcessor
{
    private readonly ILogger _logger;
    private readonly IOrderRepository _repository;

    public OrderProcessor(ILogger logger, IOrderRepository repository)
    {
        _logger = logger;
        _repository = repository;
    }

    public void Process(Order order)
    {
        // Dependencies are clear and testable
    }
}

Null Safety

Null-Conditional Operators

// ❌ Multiple null checks
if (user != null && user.Address != null && user.Address.City != null)
{
    var city = user.Address.City;
}

// ✅ Null-conditional operator
var city = user?.Address?.City;

Null-Coalescing Operators

// ❌ Verbose null check
string name;
if (user.Name != null)
{
    name = user.Name;
}
else
{
    name = "Unknown";
}

// ✅ Null-coalescing
var name = user.Name ?? "Unknown";

// ✅ Null-coalescing assignment
user.Name ??= "Unknown";  // Only assigns if null

ArgumentNullException

// ❌ Verbose null check
public void Process(User user)
{
    if (user == null)
    {
        throw new ArgumentNullException(nameof(user));
    }
}

// ✅ Use ThrowIfNull (C# 10+)
public void Process(User user)
{
    ArgumentNullException.ThrowIfNull(user);
}

Null-Forgiving Without Validation

// ❌ Null-forgiving without validation
public string GetName(User? user)
{
    return user!.Name;  // Could throw NullReferenceException!
}

// ✅ Validate before using
public string GetName(User? user)
{
    ArgumentNullException.ThrowIfNull(user);
    return user.Name;  // TypeScript knows it's not null now
}

Resource Management

Missing Disposal

// ❌ Resource not disposed
public string ReadFile(string path)
{
    var reader = new StreamReader(path);
    return reader.ReadToEnd();  // StreamReader never closed!
}

// ✅ Using statement
public string ReadFile(string path)
{
    using var reader = new StreamReader(path);
    return reader.ReadToEnd();
}  // Automatically disposed

HttpClient Misuse

// ❌ Creating HttpClient per request - socket exhaustion!
public async Task<string> GetDataAsync(string url)
{
    using var client = new HttpClient();  // Creates new socket each time!
    return await client.GetStringAsync(url);
}

// ✅ Use IHttpClientFactory
public class MyService
{
    private readonly HttpClient _client;

    public MyService(IHttpClientFactory factory)
    {
        _client = factory.CreateClient("MyApi");
    }

    public async Task<string> GetDataAsync(string url)
    {
        return await _client.GetStringAsync(url);
    }
}

Modern C# Features

Pattern Matching

// ❌ Type checking with casting
if (shape.GetType() == typeof(Circle))
{
    var circle = (Circle)shape;
    return circle.Radius * circle.Radius * Math.PI;
}

// ✅ Pattern matching
if (shape is Circle circle)
{
    return circle.Radius * circle.Radius * Math.PI;
}

// ✅ Switch expression
var area = shape switch
{
    Circle c => c.Radius * c.Radius * Math.PI,
    Rectangle r => r.Width * r.Height,
    Triangle t => t.Base * t.Height / 2,
    _ => throw new ArgumentException("Unknown shape")
};

Records for DTOs

// ❌ Verbose DTO class
public class UserDto
{
    public int Id { get; init; }
    public string Name { get; init; }
    public string Email { get; init; }

    // Need to implement Equals, GetHashCode, ToString...
}

// ✅ Record (C# 9+)
public record UserDto(int Id, string Name, string Email);

// Immutable with value equality built-in
// Easy with-expression: var updated = dto with { Name = "New Name" };

Primary Constructors (C# 12)

// ❌ Verbose class with constructor
public class UserService
{
    private readonly IUserRepository _repository;
    private readonly ILogger<UserService> _logger;

    public UserService(IUserRepository repository, ILogger<UserService> logger)
    {
        _repository = repository;
        _logger = logger;
    }
}

// ✅ Primary constructor (C# 12)
public class UserService(IUserRepository repository, ILogger<UserService> logger)
{
    public async Task<User> GetUserAsync(int id)
    {
        logger.LogInformation("Getting user {Id}", id);
        return await repository.GetByIdAsync(id);
    }
}

EF Core Best Practices

AsNoTracking for Read-Only

// ❌ Tracking when not needed
var users = await _context.Users
    .Where(u => u.Active)
    .ToListAsync();  // Tracking enabled - overhead

// ✅ AsNoTracking for read-only queries
var users = await _context.Users
    .AsNoTracking()  // No change tracking overhead
    .Where(u => u.Active)
    .ToListAsync();

Explicit Loading When Needed

// ✅ Load related data only when needed
var order = await _context.Orders.FindAsync(orderId);

if (needsCustomerInfo)
{
    await _context.Entry(order)
        .Reference(o => o.Customer)
        .LoadAsync();
}

Common Pitfalls

Best Practices Summary

1. Async All The Way - Never block on async
2. async Task - Never async void except event handlers
3. ConfigureAwait(false) - In library code
4. Materialize Once - ToList() before multiple operations
5. Include() Eagerly - Avoid N+1 queries
6. AsNoTracking() - For read-only queries
7. Dispose Resources - using for IDisposable
8. Use Modern C# - Records, pattern matching, primary constructors

Parent Hub

• _backend-mastery

Part of Workflow

This skill is utilized in the following sequential workflows:

• _workflow-feature-lifecycle