## Code Review Summary

### ✅ What's Good
- [List positive aspects]

### ⚠️ Suggestions
- [Non-blocking improvements]

### ❌ Issues (Must Fix)
- [Critical issues that must be addressed]

### 📋 Checklist
- [ ] Security review passed
- [ ] Performance review passed
- [ ] Tests included
- [ ] Documentation updated

// 🟠 Fire and forget - exceptions are lost
public void DoSomething()
{
    service.SaveAsync();  // Missing await
}

// 🔴 Blocking async code - deadlock risk
public void ProcessData()
{
    var result = asyncMethod().Result;  // Deadlock!
    var result2 = asyncMethod().GetAwaiter().GetResult();  // Still blocking!
}

// 🔴 Async void - can't catch exceptions (except event handlers)
public async void ProcessData()
{
    await service.SaveAsync();  // Exception crashes app
}

// 🟡 Missing ConfigureAwait in libraries
public async Task<Data> GetDataAsync()
{
    await service.CallAsync();  // Captures context unnecessarily
    return data;
}

// 🟠 Not propagating CancellationToken
public async Task ProcessAsync()
{
    await service.LongRunningOperationAsync();  // Missing ct parameter
}

// Async all the way
public async Task DoSomethingAsync(CancellationToken ct)
{
    await this.service.SaveAsync(ct);
}

// ConfigureAwait in library code
public async Task<Data> GetDataAsync(CancellationToken ct)
{
    await this.service.CallAsync(ct).ConfigureAwait(false);
    return data;
}

// ValueTask for frequently-awaited hot paths
public ValueTask<int> GetCachedValueAsync(string key)
{
    if (this.cache.TryGetValue(key, out var value))
        return new ValueTask<int>(value);  // Synchronous completion

    return new ValueTask<int>(LoadFromDbAsync(key));  // Async path
}

// Proper CancellationToken propagation
public async Task ProcessAsync(CancellationToken ct)
{
    ct.ThrowIfCancellationRequested();
    await this.service.ProcessAsync(ct);
}

// Async event handler (only valid async void usage)
private async void OnButtonClick(object sender, EventArgs e)
{
    try
    {
        await this.ProcessDataAsync();
    }
    catch (Exception ex)
    {
        this.logger.LogError(ex, "Error in event handler");
    }
}

// 🔴 N+1 query - database hit per iteration
foreach (var order in orders)
{
    var customer = await this.context.Customers.FindAsync(order.CustomerId);
}

// 🟠 Loading everything from database
var employees = this.context.Employees.ToList();  // Loads all columns, all rows

// 🔴 Lazy loading enabled - hidden N+1 queries
services.AddDbContext<AppDbContext>(options =>
    options.UseLazyLoadingProxies());  // Performance killer

// 🟠 Change tracking for read-only queries
var employees = await this.context.Employees
    .Where(e => e.IsActive)
    .ToListAsync();  // Unnecessary tracking overhead

// 🟡 Exposing domain entities in API
[HttpGet]
public async Task<Employee> Get(Guid id) => await this.repository.GetAsync(id);

// 🟡 Multiple database roundtrips
await this.context.Employees.Where(e => e.DepartmentId == id).ExecuteDeleteAsync();
await this.context.SaveChangesAsync();  // Already executed

// Eager loading - single query with JOIN
var orders = await this.context.Orders
    .Include(o => o.Customer)
    .ThenInclude(c => c.Address)
    .ToListAsync(ct);

// Projection with AsNoTracking for read-only
var dtos = await this.context.Employees
    .AsNoTracking()
    .Where(e => e.IsActive)
    .Select(e => new EmployeeDto(e.Id, e.Name, e.Email))
    .ToListAsync(ct);

// Batch operations (EF Core 7+)
await this.context.Employees
    .Where(e => e.DepartmentId == departmentId)
    .ExecuteUpdateAsync(
        setters => setters.SetProperty(e => e.IsActive, false),
        ct);

// Split query for multiple collections (avoid cartesian explosion)
var employees = await this.context.Employees
    .Include(e => e.Skills)
    .Include(e => e.Certifications)
    .AsSplitQuery()
    .ToListAsync(ct);

// Compiled queries for frequently-executed queries
private static readonly Func<AppDbContext, Guid, Task<Employee?>> GetEmployeeById =
    EF.CompileAsyncQuery((AppDbContext context, Guid id) =>
        context.Employees.FirstOrDefault(e => e.Id == id));

public async Task<Employee?> GetByIdAsync(Guid id, CancellationToken ct)
{
    return await GetEmployeeById(this.context, id);
}

// Using DTOs to prevent over-fetching
[HttpGet]
public async Task<EmployeeDto> Get(Guid id, CancellationToken ct)
{
    var dto = await this.context.Employees
        .AsNoTracking()
        .Where(e => e.Id == id)
        .Select(e => new EmployeeDto(e.Id, e.Name, e.Email))
        .FirstOrDefaultAsync(ct);

    return dto ?? throw new NotFoundException(nameof(Employee), id);
}

// 🟡 String interpolation - not structured, performance cost
logger.LogError($"Error processing user {userId}: {ex.Message}");

// 🟡 Concatenation - same issues
logger.LogInformation("Processing " + orderId + " for user " + userId);

// 🔴 Logging sensitive data
logger.LogInformation($"User password: {password}");  // PII violation
logger.LogDebug($"Credit card: {creditCard}");  // Security violation

// 🟠 Missing exception parameter
logger.LogError("Database error: " + ex.Message);  // Loses stack trace

// 🟡 Wrong log levels
logger.LogInformation("CRITICAL FAILURE!");  // Should be LogCritical
logger.LogError("User clicked button");  // Should be LogDebug/LogTrace

// Structured logging with parameters
logger.LogInformation("Processing order {OrderId} for user {UserId}", orderId, userId);

// Exception with message template
logger.LogError(ex, "Error processing user {UserId}", userId);

// Appropriate log levels
logger.LogTrace("Entering method GetEmployeeById with id {EmployeeId}", id);
logger.LogDebug("Cache miss for key {CacheKey}", key);
logger.LogInformation("Employee {EmployeeId} created successfully", employeeId);
logger.LogWarning("Retry attempt {Attempt} for operation {Operation}", attempt, operation);
logger.LogError(ex, "Failed to process order {OrderId}", orderId);
logger.LogCritical(ex, "Database connection lost");

// High-performance logging with LoggerMessage
private static readonly Action<ILogger, Guid, Exception?> LogEmployeeCreated =
    LoggerMessage.Define<Guid>(
        LogLevel.Information,
        new EventId(1, nameof(LogEmployeeCreated)),
        "Employee created with ID {EmployeeId}");

public async Task<Guid> Handle(CreateEmployeeCommand request, CancellationToken ct)
{
    var employee = new Employee(request.Name);
    this.context.Employees.Add(employee);
    await this.context.SaveChangesAsync(ct);

    LogEmployeeCreated(this.logger, employee.Id, null);
    return employee.Id;
}

// Correlation IDs for request tracking
using (this.logger.BeginScope(new Dictionary<string, object>
{
    ["CorrelationId"] = HttpContext.TraceIdentifier,
    ["UserId"] = User.FindFirst(ClaimTypes.NameIdentifier)?.Value
}))
{
    logger.LogInformation("Processing request");
}

// ❌ 🟡 Bad: Magic numbers
if (status == 3) { }

// ✅ Good: Named constants or enums
if (status == OrderStatus.Completed) { }

// ❌ 🟠 Bad: Catching all exceptions
try { } catch (Exception) { }

// ✅ Good: Specific exception handling
try { } catch (DbUpdateException ex) { this.logger.LogError(ex, "..."); }