Skip to content

Code Review Checklist

Key Points

  • Senior code review is a teaching + safety net + culture-shaping activity — not bureaucratic gate.
  • Two passes: design (does this approach make sense?) → details (correctness, edges, naming).
  • Distinguish blocking vs nit: state explicitly. "blocking: this races" vs "nit: prefer LINQ here".
  • Be kind, specific, actionable. "Why?" before "no". Suggest rewrites in code blocks.
  • Async-first reviews by default; sync calls reserved for sensitive / new-team-member changes.

What to look for (in order)

Pass 1: Design

  1. Is this the right approach? Could be a simpler design.
  2. Does it match the architecture? (vertical slice, ports/adapters, etc.)
  3. Is it consistent with existing code? Same naming, patterns, abstractions.
  4. Could it have been smaller? Big PRs invite review fatigue.
  5. Are tests appropriate? Coverage of the new behavior + edge cases.

Pass 2: Details

  1. Naming: clear, intention-revealing.
  2. Async correctness: cancellation tokens, ConfigureAwait if library, no blocking calls.
  3. Resource disposal: using, await using, IDisposable handled.
  4. Errors: caught at the right layer; logged with context; not swallowed.
  5. Edges: empty input, null, very large, concurrent.
  6. Security: input validation, auth check, no secrets in code, OWASP basics.
  7. Performance: O(n²) on hot paths, allocations in loops.
  8. Logging: structured, no PII, not noisy.
  9. Migrations / breaking changes: backwards-compatibility path.
  10. Documentation: docstring-worthy public APIs documented.

How to comment

🚨 BLOCKING: this races on _items; use ConcurrentBag or lock.

❓ QUESTION: does this handle the empty case? I'd expect tests to fail if `items` is null.

💡 SUGGESTION (non-blocking): you could simplify with `items.Where(...).ToList()`.

🌟 NIT: prefer `var` here for consistency with the rest of the file.

🎉 PRAISE: nice handling of the cancellation case!

Mark severity. The "no but kind" tone is critical.

Asking for changes

Before "no": - Ask why. "What was the constraint that led you to X?" - Suggest, don't dictate. "Could we...?" - Provide an example. Code blocks > prose for showing what you mean.

Instead of:
"This is wrong, use a Dictionary."

Prefer:
"For lookup-by-id, Dictionary<int, T> would be O(1) vs the List's O(n). Worth changing? Something like:

    var byId = items.ToDictionary(i => i.Id);
"

Authoring side

- PR < 400 LOC ideally.
- Description: what + why + screenshots / steps to test.
- Self-review BEFORE asking others.
- Tag specific reviewers (avoid "anyone").
- Respond to all comments (resolve or push back).

Reviewing AI-generated code

- Did the human verify it works?
- Idiomatic to YOUR codebase?
- Tests pass + reviewed by human?
- Security implications?

Don't rubber-stamp because "AI wrote it".

Async-first cadence

Default: async (PR + comments + email/Slack). 24-hour SLA.

For sensitive / large changes: sync review (call + screen share).

Common patterns to call out

Smell Comment
.Result / .Wait() "blocking: sync over async; use await"
Missing using "blocking: resource leak"
Unbounded LINQ "perf: could be O(n²)"
Catch all + log + swallow "blocking: lost error"
String SQL concat "🚨 SECURITY: SQL injection"
Hardcoded secret "🚨 SECURITY: secret in code"
Missing validation "blocking: untrusted input"
Magic numbers "nit: extract const"

What seniors don't waste time on

  • Style nits caught by formatter: let dotnet format win.
  • Subjective preferences without data: don't impose taste.
  • Bikeshedding: identify; defer to author.

Mentoring through review

  • Praise good patterns explicitly.
  • Link to docs for principles ("see ADR-12 for our error-handling stance").
  • Code reviews are tutorials in disguise.

Reviewing across seniority

Senior reviewing junior:    teaching mode; explain WHY.
Senior reviewing senior:    sparring partner; assume context.
Junior reviewing senior:    encourage; mark "I think but unsure".

When to escalate

  • Repeated quality issues from author → 1:1.
  • Architectural disagreement → design discussion separately.
  • Security blocker → escalate to security lead.

Tools

  • GitHub PR / Azure DevOps / GitLab review UIs.
  • IDE extensions for inline review.
  • CodeQL / SonarQube for static analysis (catches the boring stuff).

Senior considerations

  • Reviews are highest-leverage activity: better than your own code's impact.
  • Quality bar uniformly applied: don't approve "because it's friend".
  • Track patterns: if same comment 5 times, write a doc.
  • Be kind first, technical second: tone makes or breaks team culture.

Cross-references