My Code Review Checklist — After 13 Years of Delivery

After 13+ years building enterprise systems — US payroll, supply chain, healthcare, e-commerce — I've reviewed hundreds of PRs. This is the checklist I run through every single time. No fluff, no theory. Every item maps to a real production incident.

Code reviews are one of the highest-leverage activities on any engineering team. A 30-minute review that catches a missing WHERE clause on an UPDATE statement can save you hours of production firefighting. But most developers don't have a structured way to think about them — they eyeball the diff, check a few things, and approve.

The six areas I check

Every PR goes through these six lenses in order. Requirements first — there's no point checking code quality if the code doesn't solve the right problem.

// ? Symptom fix — masks the bug everywhere else it's consumed
var total = order.Subtotal + Math.Abs(discount);  // hides negative discount

// ? Root cause fix — correct the sign in PricingService
public decimal ApplyDiscount(decimal price, decimal discountPct)
    => price - (price * discountPct / 100);  // always returns positive reduction

// ? Tax calculation inside a controller action
[HttpGet("salary/{id}")]
public IActionResult GetSalary(int id) {
    var emp  = _db.Employees.Find(id);
    var tax  = emp.Salary * 0.30m;   // tax logic here = untestable, unreusable
    var net  = emp.Salary - tax;
    return Ok(new { net });
}

// ? Controller is thin — logic lives in PayrollService
[HttpGet("salary/{id}")]
public IActionResult GetSalary(int id) {
    var result = _payrollService.GetNetSalary(id);
    return Ok(result);
}

// ? Dev writes a custom date formatter from scratch
public string FormatDate(DateTime d)
    => $"{d.Day:D2}/{d.Month:D2}/{d.Year}";

// ? DateHelper.ToDisplayDate() already exists in /Common/DateHelper.cs
public string FormatDate(DateTime d)
    => DateHelper.ToDisplayDate(d);  // consistent across the whole platform

// ? Connection string in source — now in git history forever
var client = new ServiceBusClient(
    "Endpoint=sb://prod.servicebus.windows.net/;SharedAccessKey=xK3mP9...");

// ? Endpoint has no [Authorize] — anyone can call it
[HttpGet("employees/payroll-data")]
public IActionResult GetPayrollData() { ... }

// ? Secret from config, endpoint locked down
var client = new ServiceBusClient(_config["ServiceBus:ConnectionString"]);

[Authorize(Policy = "PayrollAdmin")]
[HttpGet("employees/payroll-data")]
public IActionResult GetPayrollData() { ... }

// ? SSN written to application log — a compliance incident waiting to happen
_logger.LogInformation("Processing employee SSN: {ssn}", employee.SSN);
_logger.LogDebug("Token: {tok}", authToken);

// ? Log reference IDs only — never the sensitive values themselves
_logger.LogInformation("Processing payroll record: {id}", employee.Id);
_logger.LogDebug("Auth flow completed for user: {id}", userId);

// ? String concatenation — classic injection vector
var sql = "SELECT * FROM Employees WHERE Department = '"
          + deptName + "'";   // deptName = "'; DROP TABLE Employees;--"

// ? Always use parameterised queries
var emps = _db.Employees
    .Where(e => e.Department == deptName)
    .ToList();

// Or with Dapper:
var emps = conn.Query<Employee>(
    "SELECT * FROM Employees WHERE Department = @dept",
    new { dept = deptName });

// ? 1 query for departments, then 1 per department — 51 DB hits for 50 depts
var departments = _db.Departments.ToList();
foreach (var dept in departments) {
    dept.Employees = _db.Employees
        .Where(e => e.DeptId == dept.Id)
        .ToList();
}

// ? Single query with eager load
var departments = _db.Departments
    .Include(d => d.Employees)
    .ToList();

// ? .Result blocks the thread — under load this deadlocks ASP.NET
var report = _reportService.GenerateAsync(month).Result;
_emailService.SendAsync(report).Wait();

// ? Properly awaited — thread is freed while I/O runs
var report = await _reportService.GenerateAsync(month);
await _emailService.SendAsync(report);

// ? Subscription never cleaned up — leaks on every route navigation
export class EmployeeListComponent {
  ngOnInit() {
    this.employeeService.getAll().subscribe(data => this.employees = data);
  }
}

// ? Use takeUntilDestroyed (Angular 16+) or manual unsubscribe
export class EmployeeListComponent implements OnDestroy {
  private destroy$ = new Subject<void>();

  ngOnInit() {
    this.employeeService.getAll()
      .pipe(takeUntil(this.destroy$))
      .subscribe(data => this.employees = data);
  }

  ngOnDestroy() { this.destroy$.next(); this.destroy$.complete(); }
}

// ? Silent catch — error disappears, nobody knows why payroll broke
try {
    await ProcessPayrollAsync(employeeId);
} catch (Exception) { }   // TODO: handle later (it never gets handled)

// ? Magic number — what does 3 mean?
if (employee.Status == 3) { ... }

// ? Log + rethrow so the error surfaces
try {
    await ProcessPayrollAsync(employeeId);
} catch (Exception ex) {
    _logger.LogError(ex, "Payroll failed for employee {Id}", employeeId);
    throw;
}

// ? Self-documenting enum
if (employee.Status == EmployeeStatus.OnLeave) { ... }

// ? manager can be null if employee has no manager assigned
var managerName = employee.Manager.FullName;   // NullReferenceException in prod

// ? Null-conditional + fallback
var managerName = employee.Manager?.FullName ?? "Unassigned";

// ? Old commented block + console.log left in Angular component
// var oldCalc = base * 1.18;
// return oldCalc;
console.log('DEBUG employee data:', this.employee);
return base * this.taxRate;

// ? Clean — only living code, no noise
return base * this.taxRate;

-- ? This updates EVERY row in the Employees table
UPDATE Employees SET Salary = @NewSalary

-- ? Always scope writes to the intended rows
UPDATE Employees
SET    Salary = @NewSalary
WHERE  Id = @EmployeeId

-- ? Two writes, no transaction — if the INSERT fails, UPDATE already committed
CREATE PROCEDURE UpdateEmployeeSalary
    @EmployeeId INT, @NewSalary DECIMAL(18,2)
AS BEGIN
    UPDATE Employees SET Salary = @NewSalary WHERE Id = @EmployeeId
    INSERT INTO AuditLog (EmployeeId, Action) VALUES (@EmployeeId, 'SALARY_CHANGE')
END

-- ? Atomic: both succeed or both roll back
CREATE PROCEDURE UpdateEmployeeSalary
    @EmployeeId INT, @NewSalary DECIMAL(18,2)
AS BEGIN
    SET NOCOUNT ON;
    BEGIN TRANSACTION;
    BEGIN TRY
        UPDATE Employees SET Salary = @NewSalary WHERE Id = @EmployeeId
        INSERT INTO AuditLog (EmployeeId, Action, ChangedAt)
        VALUES (@EmployeeId, 'SALARY_CHANGE', GETUTCDATE())
        COMMIT TRANSACTION;
    END TRY
    BEGIN CATCH
        ROLLBACK TRANSACTION;
        THROW;
    END CATCH
END

-- ? Cursor: processes 50,000 rows one at a time — takes minutes
DECLARE @Id INT
DECLARE cur CURSOR FOR SELECT Id FROM Employees WHERE IsActive = 1
OPEN cur
FETCH NEXT FROM cur INTO @Id
WHILE @@FETCH_STATUS = 0 BEGIN
    UPDATE Employees SET LastReviewed = GETUTCDATE() WHERE Id = @Id
    FETCH NEXT FROM cur INTO @Id
END
CLOSE cur; DEALLOCATE cur;

-- ? Set-based: single statement, same result, milliseconds
UPDATE Employees
SET    LastReviewed = GETUTCDATE()
WHERE  IsActive = 1

Why requirements come first

The biggest mistake junior developers make in code reviews is jumping straight to code style — naming, formatting, structure. Those things matter, but they're secondary to the most important question: does this code actually solve the right problem?

I've seen beautifully structured, well-named, zero-lint-error PRs that completely missed the acceptance criteria of the ticket. Clean code solving the wrong problem is worse than messy code solving the right one — it ships to production and the bug report comes back two weeks later with more context to untangle.

The security mindset — especially on payroll systems

When you're working on systems that touch real money — payroll, invoicing, tax calculations — security reviews aren't optional hygiene. A missing authorization check on a payroll endpoint isn't a "medium severity" finding. It's a potential compliance incident.

At Cast & Crew, we had Snyk, Checkmarx, Trace3 Pen Test, and CAST running against our codebase. I eliminated 70% of flagged vulnerabilities across the platform. The pattern I saw most often: hardcoded credentials, missing endpoint authorization, and secrets in appsettings committed to the repo.

How I explain this in interviews

The meta-principle

A code review is not a style audit. It's the last engineering checkpoint before code goes to production and becomes someone else's problem at 2am.

Every item in the checklist above maps to a real class of production incident I've either seen happen or caught before it shipped. Requirements coverage ? wrong behavior. Missing WHERE clause ? data corruption. No auth check ? security incident. N+1 query ? timeouts under load. Swallowed exception ? silent data loss.

The faster your team internalizes these patterns, the fewer production incidents you have — and fewer incidents means more time building features, which is what everyone actually wants.