# .NET Code Review Checklist

Use this checklist to systematically evaluate .NET code changes. Items are ordered by risk severity.

---

## 1. Correctness and Data Integrity

- [ ] **Logic correctness** - Does the code produce the intended result for all inputs?
- [ ] **Edge cases** - Are null, empty, boundary, and exceptional inputs handled?
- [ ] **Data mutations** - Are collections modified safely during iteration?
- [ ] **Race conditions** - Can concurrent access corrupt shared state?
- [ ] **Transactions** - Are multi-step data changes wrapped in appropriate transactions?
- [ ] **Idempotency** - Are retryable operations safe to execute multiple times?

## 2. Concurrency and Async

- [ ] **Async all the way** - No `Task.Result`, `.Wait()`, or `.GetAwaiter().GetResult()` in async contexts?
- [ ] **ConfigureAwait** - Is `ConfigureAwait(false)` used appropriately in library code?
- [ ] **Cancellation** - Are `CancellationToken` parameters passed through the call chain?
- [ ] **Deadlocks** - Could synchronous waits on UI or ASP.NET synchronization contexts deadlock?
- [ ] **Thread safety** - Are shared resources protected with locks, `Interlocked`, or concurrent collections?
- [ ] **Parallel pitfalls** - Do `Parallel.ForEach` or `Task.WhenAll` calls handle exceptions correctly?

## 3. Resource Lifecycle and Disposal

- [ ] **IDisposable** - Are disposable objects disposed via `using` or explicit disposal?
- [ ] **IAsyncDisposable** - Are async-disposable resources handled with `await using`?
- [ ] **Scope lifetime** - Do DI-registered services have appropriate lifetimes (transient, scoped, singleton)?
- [ ] **HttpClient reuse** - Is `HttpClient` reused via `IHttpClientFactory` to avoid socket exhaustion?
- [ ] **Event handlers** - Are event subscriptions unsubscribed to prevent memory leaks?
- [ ] **Finalizers** - If implementing a finalizer, is the dispose pattern followed correctly?

## 4. Security

- [ ] **Input validation** - Are user inputs validated and sanitized?
- [ ] **SQL injection** - Are parameterized queries or EF used instead of string concatenation?
- [ ] **XSS** - Are outputs HTML-encoded in web contexts?
- [ ] **Secrets** - Are credentials, keys, and tokens stored securely (not in code or config)?
- [ ] **Authorization** - Are authorization checks performed before sensitive operations?
- [ ] **CSRF** - Are anti-forgery tokens used for state-changing operations?
- [ ] **Path traversal** - Are file paths validated to prevent directory traversal attacks?

## 5. Exception Handling

- [ ] **Specific exceptions** - Are specific exception types caught instead of bare `catch (Exception)`?
- [ ] **Exception swallowing** - Are exceptions logged or rethrown, not silently swallowed?
- [ ] **Exception wrapping** - Is the original exception preserved via inner exception when rethrowing?
- [ ] **Validation vs exception** - Are expected failures handled via validation, not exceptions?
- [ ] **Finally blocks** - Do critical cleanup operations use `finally` or `using`?

## 6. API Design and Breaking Changes

- [ ] **Binary compatibility** - Do changes preserve binary compatibility for library consumers?
- [ ] **Behavioral compatibility** - Do changes maintain expected behavior for existing callers?
- [ ] **Nullability annotations** - Are nullability annotations accurate and consistent?
- [ ] **Default parameters** - Do new default parameter values make sense for existing callers?
- [ ] **Obsolete members** - Are deprecated members marked with `[Obsolete]` and guidance provided?

## 7. Performance

- [ ] **Allocations** - Are unnecessary allocations avoided in hot paths?
- [ ] **String operations** - Is `StringBuilder` used for multiple concatenations?
- [ ] **LINQ in loops** - Are LINQ queries evaluated once, not per iteration?
- [ ] **Lazy evaluation** - Is deferred execution understood and intentional?
- [ ] **Boxing** - Is boxing avoided for value types in performance-critical code?
- [ ] **Span/Memory** - Are `Span<T>` or `Memory<T>` used for buffer operations?

## 8. Entity Framework and Data Access

- [ ] **N+1 queries** - Are related entities eagerly loaded when needed?
- [ ] **Tracking** - Is `AsNoTracking()` used for read-only queries?
- [ ] **Client evaluation** - Are LINQ expressions translatable to SQL?
- [ ] **Migrations** - Do migrations handle existing data correctly?
- [ ] **Connection management** - Are database connections scoped appropriately?
- [ ] **Bulk operations** - Are bulk inserts/updates used for large data sets?

## 9. ASP.NET Core Specifics

- [ ] **Middleware order** - Is middleware registered in the correct order?
- [ ] **Model binding** - Are model binding errors handled appropriately?
- [ ] **Action filters** - Are cross-cutting concerns handled via filters, not repeated code?
- [ ] **Response caching** - Are cache headers set correctly for cacheable responses?
- [ ] **Request size limits** - Are large request body limits configured appropriately?

## 10. Testing

- [ ] **Test coverage** - Are new code paths covered by tests?
- [ ] **Edge case tests** - Do tests cover boundary conditions and error cases?
- [ ] **Test isolation** - Are tests independent and not reliant on execution order?
- [ ] **Mock boundaries** - Are external dependencies mocked at appropriate boundaries?
- [ ] **Assertion clarity** - Do test assertions clearly indicate what is being verified?

## 11. Observability

- [ ] **Logging** - Are significant operations and errors logged appropriately?
- [ ] **Log levels** - Are log levels (Debug, Info, Warning, Error) used correctly?
- [ ] **Structured logging** - Are log messages structured with named parameters?
- [ ] **Metrics** - Are key business and performance metrics captured?
- [ ] **Tracing** - Is distributed tracing context propagated?

## 12. Documentation and Maintainability

- [ ] **XML documentation** - Are public APIs documented with XML comments?
- [ ] **Code comments** - Do comments explain "why", not "what"?
- [ ] **Naming clarity** - Are names descriptive and consistent with conventions?
- [ ] **Complexity** - Is cyclomatic complexity reasonable?
- [ ] **Dead code** - Is unused code removed rather than commented out?

---

## Quick Reference: Review Priority

| Priority | Category | Risk Level |
|----------|----------|------------|
| P0 | Data loss, security vulnerabilities, crashes | Critical |
| P1 | Concurrency bugs, resource leaks, broken functionality | High |
| P2 | Performance regressions, missing error handling | Medium |
| P3 | API design issues, missing tests | Medium |
| P4 | Code style, documentation gaps | Low |

Focus review effort on P0-P2 issues before addressing P3-P4 concerns.

# .NET Code Review Patterns

This reference covers common patterns and anti-patterns for async code, resource disposal, and security in .NET code reviews.

---

## Async Patterns

### Pattern: Async All the Way

Async should propagate through the entire call chain. Blocking on async code causes deadlocks and thread pool starvation.

```csharp
// GOOD: Async propagates through the call chain
public class OrderService(IOrderRepository repository, IPaymentGateway paymentGateway)
{
    public async Task<OrderResult> ProcessOrderAsync(Order order, CancellationToken cancellationToken = default)
    {
        var validated = await ValidateOrderAsync(order, cancellationToken);
        var payment = await paymentGateway.ChargeAsync(order.Total, cancellationToken);
        return await repository.SaveOrderAsync(validated, payment, cancellationToken);
    }
}

// BAD: Blocking on async - causes deadlocks in UI/ASP.NET contexts
public class OrderService(IOrderRepository repository, IPaymentGateway paymentGateway)
{
    public OrderResult ProcessOrder(Order order)
    {
        // NEVER DO THIS - deadlock risk
        var validated = ValidateOrderAsync(order).Result;
        var payment = paymentGateway.ChargeAsync(order.Total).GetAwaiter().GetResult();
        return repository.SaveOrderAsync(validated, payment).Wait();
    }
}
```

### Pattern: Cancellation Token Propagation

Always accept and propagate `CancellationToken` to support cooperative cancellation.

```csharp
// GOOD: CancellationToken flows through all async operations
public class DataProcessor(HttpClient httpClient, ILogger<DataProcessor> logger)
{
    public async Task<ProcessedData> FetchAndProcessAsync(
        string url,
        CancellationToken cancellationToken = default)
    {
        var response = await httpClient.GetAsync(url, cancellationToken);
        response.EnsureSuccessStatusCode();

        var content = await response.Content.ReadAsStringAsync(cancellationToken);

        return await ProcessContentAsync(content, cancellationToken);
    }

    private async Task<ProcessedData> ProcessContentAsync(
        string content,
        CancellationToken cancellationToken)
    {
        cancellationToken.ThrowIfCancellationRequested();
        // Processing logic...
        return await Task.FromResult(new ProcessedData(content));
    }
}

// BAD: Ignoring cancellation
public async Task<ProcessedData> FetchAndProcessAsync(string url)
{
    // No way to cancel this operation
    var response = await httpClient.GetAsync(url);
    var content = await response.Content.ReadAsStringAsync();
    return await ProcessContentAsync(content);
}
```

### Pattern: ConfigureAwait in Libraries

Library code should use `ConfigureAwait(false)` to avoid capturing synchronization context.

```csharp
// GOOD: Library code avoids context capture
public class CacheService(IDistributedCache cache)
{
    public async Task<T?> GetOrCreateAsync<T>(
        string key,
        Func<Task<T>> factory,
        CancellationToken cancellationToken = default) where T : class
    {
        var cached = await cache.GetStringAsync(key, cancellationToken).ConfigureAwait(false);
        if (cached is not null)
        {
            return JsonSerializer.Deserialize<T>(cached);
        }

        var value = await factory().ConfigureAwait(false);
        var json = JsonSerializer.Serialize(value);
        await cache.SetStringAsync(key, json, cancellationToken).ConfigureAwait(false);
        return value;
    }
}
```

### Pattern: Proper Exception Handling in Async

Handle exceptions at appropriate boundaries without losing stack traces.

```csharp
// GOOD: Proper async exception handling
public class RetryingService(IExternalApi api, ILogger<RetryingService> logger)
{
    public async Task<Result> ExecuteWithRetryAsync(
        Request request,
        int maxRetries = 3,
        CancellationToken cancellationToken = default)
    {
        var attempt = 0;
        while (true)
        {
            try
            {
                return await api.CallAsync(request, cancellationToken);
            }
            catch (HttpRequestException ex) when (attempt < maxRetries)
            {
                attempt++;
                logger.LogWarning(ex, "Attempt {Attempt} failed, retrying...", attempt);
                await Task.Delay(TimeSpan.FromSeconds(Math.Pow(2, attempt)), cancellationToken);
            }
            catch (OperationCanceledException)
            {
                logger.LogInformation("Operation cancelled");
                throw; // Don't wrap cancellation exceptions
            }
        }
    }
}
```

### Anti-Pattern: Async Void

Never use `async void` except for event handlers. Exceptions cannot be caught.

```csharp
// BAD: async void - exceptions are unobservable
public async void ProcessDataBad(Data data)
{
    await SaveAsync(data); // If this throws, the exception is lost
}

// GOOD: Return Task for proper exception handling
public async Task ProcessDataAsync(Data data)
{
    await SaveAsync(data);
}

// ACCEPTABLE: Event handlers only
private async void Button_Click(object sender, EventArgs e)
{
    try
    {
        await ProcessDataAsync(GetData());
    }
    catch (Exception ex)
    {
        ShowError(ex.Message);
    }
}
```

---

## Disposal Patterns

### Pattern: Using Declarations and Statements

Use `using` to ensure disposal even when exceptions occur.

```csharp
// GOOD: Using declaration (C# 8+)
public async Task ProcessFileAsync(string path, CancellationToken cancellationToken = default)
{
    await using var stream = File.OpenRead(path);
    await using var reader = new StreamReader(stream);

    var content = await reader.ReadToEndAsync(cancellationToken);
    await ProcessContentAsync(content, cancellationToken);
}

// GOOD: Using statement for explicit scope
public async Task<byte[]> DownloadAsync(string url, CancellationToken cancellationToken = default)
{
    using (var response = await _httpClient.GetAsync(url, cancellationToken))
    {
        response.EnsureSuccessStatusCode();
        return await response.Content.ReadAsByteArrayAsync(cancellationToken);
    }
}

// BAD: Manual disposal is error-prone
public async Task ProcessFileBadAsync(string path)
{
    var stream = File.OpenRead(path);
    var reader = new StreamReader(stream);

    var content = await reader.ReadToEndAsync();
    await ProcessContentAsync(content);

    reader.Dispose(); // May not be reached if exception occurs
    stream.Dispose();
}
```

### Pattern: IAsyncDisposable

Use `await using` for types implementing `IAsyncDisposable`.

```csharp
// GOOD: Async disposal for async resources
public class AsyncDatabaseConnection : IAsyncDisposable
{
    private readonly SqlConnection _connection;

    public AsyncDatabaseConnection(string connectionString)
    {
        _connection = new SqlConnection(connectionString);
    }

    public async Task OpenAsync(CancellationToken cancellationToken = default)
    {
        await _connection.OpenAsync(cancellationToken);
    }

    public async ValueTask DisposeAsync()
    {
        await _connection.DisposeAsync();
    }
}

// Usage
public async Task QueryDataAsync(CancellationToken cancellationToken = default)
{
    await using var connection = new AsyncDatabaseConnection(_connectionString);
    await connection.OpenAsync(cancellationToken);
    // Use connection...
}
```

### Pattern: HttpClient via IHttpClientFactory

Never create `HttpClient` instances directly in application code.

```csharp
// GOOD: Use IHttpClientFactory to avoid socket exhaustion
public class ApiClient(IHttpClientFactory httpClientFactory)
{
    public async Task<ApiResponse> GetDataAsync(
        string endpoint,
        CancellationToken cancellationToken = default)
    {
        using var client = httpClientFactory.CreateClient("ExternalApi");
        var response = await client.GetAsync(endpoint, cancellationToken);
        return await response.Content.ReadFromJsonAsync<ApiResponse>(cancellationToken: cancellationToken);
    }
}

// Registration in DI
services.AddHttpClient("ExternalApi", client =>
{
    client.BaseAddress = new Uri("https://api.example.com/");
    client.DefaultRequestHeaders.Add("Accept", "application/json");
});

// BAD: Direct HttpClient creation causes socket exhaustion
public class ApiClientBad
{
    public async Task<ApiResponse> GetDataAsync(string endpoint)
    {
        // NEVER DO THIS - leads to socket exhaustion
        using var client = new HttpClient();
        var response = await client.GetAsync($"https://api.example.com/{endpoint}");
        return await response.Content.ReadFromJsonAsync<ApiResponse>();
    }
}
```

### Pattern: Unsubscribe Event Handlers

Prevent memory leaks by unsubscribing from events.

```csharp
// GOOD: Unsubscribe in Dispose
public class DataMonitor(IEventSource eventSource) : IDisposable
{
    private bool _disposed;

    public void StartMonitoring()
    {
        eventSource.DataReceived += OnDataReceived;
    }

    private void OnDataReceived(object? sender, DataEventArgs e)
    {
        // Handle event...
    }

    public void Dispose()
    {
        if (_disposed) return;

        eventSource.DataReceived -= OnDataReceived;
        _disposed = true;
    }
}
```

### Pattern: DI Lifetime Management

Choose appropriate service lifetimes to avoid captured dependencies and memory leaks.

```csharp
// Registration
services.AddSingleton<ICacheService, CacheService>();     // One instance for app lifetime
services.AddScoped<IUserContext, UserContext>();          // One instance per request
services.AddTransient<IValidator, Validator>();           // New instance each time

// BAD: Singleton captures scoped dependency - memory leak and incorrect behavior
public class BadSingletonService(IUserContext userContext) // Scoped captured in singleton!
{
    // userContext will be the same for all requests
}

// GOOD: Use IServiceScopeFactory for scoped access in singletons
public class GoodSingletonService(IServiceScopeFactory scopeFactory)
{
    public async Task DoWorkAsync(CancellationToken cancellationToken = default)
    {
        using var scope = scopeFactory.CreateScope();
        var userContext = scope.ServiceProvider.GetRequiredService<IUserContext>();
        // Use userContext...
    }
}
```

---

## Security Patterns

### Pattern: Parameterized Queries

Always use parameterized queries to prevent SQL injection.

```csharp
// GOOD: Parameterized query via Dapper
public class UserRepository(IDbConnection connection)
{
    public async Task<User?> GetByIdAsync(int userId, CancellationToken cancellationToken = default)
    {
        var command = new CommandDefinition(
            "SELECT Id, Name, Email FROM Users WHERE Id = @Id",
            new { Id = userId },
            cancellationToken: cancellationToken);

        return await connection.QuerySingleOrDefaultAsync<User>(command);
    }
}

// GOOD: Entity Framework handles parameterization
public class UserRepository(AppDbContext context)
{
    public async Task<User?> GetByEmailAsync(string email, CancellationToken cancellationToken = default)
    {
        return await context.Users
            .FirstOrDefaultAsync(u => u.Email == email, cancellationToken);
    }
}

// BAD: String interpolation = SQL injection vulnerability
public async Task<User?> GetByIdBadAsync(int userId)
{
    // NEVER DO THIS
    var sql = $"SELECT * FROM Users WHERE Id = {userId}";
    return await connection.QuerySingleOrDefaultAsync<User>(sql);
}
```

### Pattern: Input Validation

Validate all inputs at system boundaries.

```csharp
// GOOD: Comprehensive input validation
public class CreateUserCommand
{
    [Required]
    [StringLength(100, MinimumLength = 2)]
    public required string Name { get; init; }

    [Required]
    [EmailAddress]
    [StringLength(254)]
    public required string Email { get; init; }

    [Required]
    [MinLength(12)]
    [RegularExpression(@"^(?=.*[a-z])(?=.*[A-Z])(?=.*\d).*$",
        ErrorMessage = "Password must contain uppercase, lowercase, and number")]
    public required string Password { get; init; }
}

public class UserService(IValidator<CreateUserCommand> validator)
{
    public async Task<Result<User>> CreateUserAsync(
        CreateUserCommand command,
        CancellationToken cancellationToken = default)
    {
        var validation = await validator.ValidateAsync(command, cancellationToken);
        if (!validation.IsValid)
        {
            return Result<User>.Failure(validation.Errors);
        }

        // Proceed with creation...
    }
}
```

### Pattern: Path Traversal Prevention

Validate file paths to prevent directory traversal attacks.

```csharp
// GOOD: Validate paths are within allowed directory
public class FileService(IWebHostEnvironment environment, ILogger<FileService> logger)
{
    public async Task<byte[]> GetFileAsync(string fileName, CancellationToken cancellationToken = default)
    {
        // Sanitize filename
        var sanitizedName = Path.GetFileName(fileName);
        if (string.IsNullOrEmpty(sanitizedName) || sanitizedName != fileName)
        {
            throw new ArgumentException("Invalid filename", nameof(fileName));
        }

        var uploadsPath = Path.Combine(environment.WebRootPath, "uploads");
        var filePath = Path.GetFullPath(Path.Combine(uploadsPath, sanitizedName));

        // Ensure resolved path is within uploads directory
        if (!filePath.StartsWith(uploadsPath + Path.DirectorySeparatorChar))
        {
            logger.LogWarning("Path traversal attempt detected: {FileName}", fileName);
            throw new UnauthorizedAccessException("Access denied");
        }

        return await File.ReadAllBytesAsync(filePath, cancellationToken);
    }
}

// BAD: Allows path traversal
public async Task<byte[]> GetFileBadAsync(string fileName)
{
    // NEVER DO THIS - allows ../../../etc/passwd
    var path = Path.Combine(_uploadsPath, fileName);
    return await File.ReadAllBytesAsync(path);
}
```

### Pattern: Secret Management

Never hardcode secrets; use secure configuration.

```csharp
// GOOD: Use configuration and secret management
public class ExternalApiClient(IOptions<ApiSettings> options, HttpClient httpClient)
{
    public async Task<Response> CallApiAsync(Request request, CancellationToken cancellationToken = default)
    {
        httpClient.DefaultRequestHeaders.Authorization =
            new AuthenticationHeaderValue("Bearer", options.Value.ApiKey);

        return await httpClient.PostAsJsonAsync("/api/endpoint", request, cancellationToken);
    }
}

// appsettings.json (development only, use user secrets or Azure Key Vault in production)
// {
//   "ApiSettings": {
//     "ApiKey": "from-user-secrets-or-keyvault"
//   }
// }

// BAD: Hardcoded secrets
public class ExternalApiClientBad(HttpClient httpClient)
{
    // NEVER DO THIS
    private const string ApiKey = "sk-abc123secretkey";

    public async Task CallApiAsync()
    {
        httpClient.DefaultRequestHeaders.Authorization =
            new AuthenticationHeaderValue("Bearer", ApiKey);
    }
}
```

### Pattern: Authorization Checks

Enforce authorization before accessing protected resources.

```csharp
// GOOD: Authorization check before sensitive operation
public class DocumentService(
    IDocumentRepository repository,
    IAuthorizationService authorizationService)
{
    public async Task<Document?> GetDocumentAsync(
        Guid documentId,
        ClaimsPrincipal user,
        CancellationToken cancellationToken = default)
    {
        var document = await repository.GetByIdAsync(documentId, cancellationToken);
        if (document is null)
        {
            return null;
        }

        var authResult = await authorizationService.AuthorizeAsync(
            user, document, "DocumentRead");

        if (!authResult.Succeeded)
        {
            throw new UnauthorizedAccessException("Access to document denied");
        }

        return document;
    }
}

// Controller with authorization attribute
[Authorize]
public class DocumentsController(IDocumentService documentService) : ControllerBase
{
    [HttpGet("{id}")]
    public async Task<ActionResult<Document>> Get(
        Guid id,
        CancellationToken cancellationToken)
    {
        var document = await documentService.GetDocumentAsync(id, User, cancellationToken);
        return document is null ? NotFound() : Ok(document);
    }
}
```

### Pattern: Output Encoding

Encode output to prevent XSS attacks.

```csharp
// GOOD: Razor automatically encodes output
// In .cshtml:
// <p>@Model.UserInput</p>  <!-- Automatically HTML-encoded -->

// GOOD: Explicit encoding when needed
public class HtmlService(HtmlEncoder encoder)
{
    public string SafeRender(string userInput)
    {
        return encoder.Encode(userInput);
    }
}

// BAD: Raw HTML output
// <p>@Html.Raw(Model.UserInput)</p>  <!-- XSS vulnerability if UserInput contains scripts -->
```

---

## Review Questions

When reviewing code for these patterns, ask:

**Async**
- Can any async call block the calling thread?
- Are all `CancellationToken` parameters propagated?
- Does library code use `ConfigureAwait(false)`?

**Disposal**
- Are all `IDisposable`/`IAsyncDisposable` resources properly disposed?
- Are event handlers unsubscribed?
- Are DI lifetimes appropriate for the dependencies?

**Security**
- Could user input reach a sensitive operation unvalidated?
- Are queries parameterized?
- Could file operations be exploited for path traversal?
- Are secrets stored securely?