Effective Code Reviews — Feedback That Improves Code

Code review serves three purposes — weighted differently by role:

Junior engineers reviewing each other:
  → Catching obvious bugs and logic errors
  → Sharing knowledge: "I didn't know you could do it that way"
  → Learning to read others' code

Senior engineers reviewing:
  → Correctness and edge cases
  → Consistency with codebase conventions
  → Identifying performance or security issues

Tech leads reviewing:
  → Architecture and design: does this fit the overall system design?
  → Mentoring: does the PR reflect growth? What would help them improve?
  → Standards: is this consistent with the team's established patterns?
  → Complexity: will the next engineer be able to maintain this?

Common tech lead mistake:
  Reviewing code the same way you reviewed as a senior engineer —
  every line, every detail, optimising for zero bugs.

The cost: slow delivery (PR blocked for 2 days), demoralised engineers
  (feels like every choice is wrong), and missed architecture issues
  (you're checking variable names and missed the wrong data model).

Review for architecture first. Comments on naming are secondary.

Review concerns in priority order:

1. Architecture and design (blocker if wrong):
   - Is this the right place for this logic? (Prescription domain logic in API controller?)
   - Does this create the right module boundaries?
   - Will this design scale? (N+1 query pattern that's fine for 10 patients, broken for 200)
   - Does this create coupling that shouldn't exist?

2. Correctness (blocker if wrong):
   - Does this handle the edge cases in the clinical domain?
   - Is data modified correctly? (Prescription status transitions — can it go Draft→Suspended?)
   - Are error paths handled?
   - Is the security model correct? (Does this require auth? Which roles?)

3. Tests (request changes if missing for non-trivial logic):
   - Is the happy path tested?
   - Are the domain rule tests present? (INR range validation, MRN format)
   - Does the test use real-ish data or vacuous constants?

4. Performance (note if suspicious, don't always block):
   - Obvious N+1 patterns
   - Missing cancellation tokens on async paths
   - Sync-over-async patterns

5. Style and conventions (comments, not blockers):
   - Naming consistency with the codebase
   - Code organisation conventions
   - Comments where intent is non-obvious

If you're spending most of your review time on level 5, you need a linter.
If you're spending most of your time on levels 1–2, you need better upfront design discussions.

The tone of code review shapes engineering culture.
A code review that makes an engineer feel incompetent creates defensive behaviour —
they argue comments rather than learn from them.
A code review that treats them as a capable engineer being challenged grows them.

For every comment, ask: what do I want this person to understand?
Then write the comment that teaches that, not the comment that just says "wrong."

BAD:  "This is wrong."
GOOD: "This will fail when the patient has no active prescriptions — the
       .First() will throw InvalidOperationException on an empty list.
       Consider .FirstOrDefault() with a null check, or check Any() first."

BAD:  "This should be in the domain layer."
GOOD: "This validation logic (INR range check for Warfarin) is business logic —
       it belongs in the Prescription domain entity, not the API controller.
       That way it's enforced everywhere the prescription is approved,
       not just via this endpoint. See how DoseValue validates in DoseValue.cs."

BAD:  "Naming is bad."
GOOD: "I'd rename @result to @approvalResult here — it took me a moment to see
       what it represented. In context there are two result variables in scope."

Label the intent of comments:
  [Blocker] → must be addressed before merge
  [Suggestion] → I'd do it differently, but this works — your call
  [Question] → I want to understand the intention here
  [Nit] → very minor style thing — feel free to ignore
  [Learning] → here's context that might be useful to know

Engineers stop reading long comments.
One clear point per comment. Multiple comments if multiple points.

A 2,000-line PR is not reviewable in a meaningful way.
Reviewing it superficially is worse than not reviewing it — it gives false confidence.

When a large PR arrives:

1. Understand why it's large
   - Feature that couldn't be split (rare, usually split is possible)
   - Engineer hasn't been taught to split PRs (most common cause)
   - Genuine refactor that's hard to split incrementally

2. For avoidable large PRs — return it with guidance:
   "This PR is too large to review meaningfully. Can we split it into:
    1. The database schema changes and migrations
    2. The repository and domain layer changes
    3. The API endpoint and integration tests
    I can review each in 30 minutes; this one would need 3 hours for a real review."

3. For unavoidable large PRs:
   - Ask the engineer to walk you through it (synchronous design review)
   - Review the architecture first — if the design is wrong, the details don't matter
   - Focus on the highest-risk areas, not every line
   - Accept that you will miss things and compensate with good test coverage

Teach small PRs as a team practice:
  "Each PR should be reviewable in 30 minutes"
  Small PRs: faster review, faster merge, smaller blast radius on problems
  Large PRs: slow review, merge conflicts, hidden design issues

The forcing function for small PRs:
  If I can't describe what this PR does in one sentence, it might be two PRs.

// Clinical domain code has higher review stakes — incorrect logic can affect patients
// Specific things to check in prescription-related code:

// 1. Status transition correctness
// Is this transition valid? (Draft → Suspended is not valid directly)
prescription.Suspend(reason);  // Review: can you suspend a Draft prescription?
                               // Answer: no — only Approved prescriptions can be suspended

// 2. INR range validation
// Is the INR range correct for the indication?
if (inr >= 2.0m && inr <= 3.0m)  // Review: is 2.0–3.0 correct for all indications?
    // Warfarin for AF: 2.0–3.0 ✓
    // Warfarin for mechanical heart valve: 2.5–3.5 — this would be wrong for that case
    // Does the code need to check the indication?

// 3. Dose boundaries
// Is the maximum dose enforced at the domain level or only at the API?
var dose = DoseValue.Of(doseAmount, unit);
// Review: what happens if doseAmount is 0 or negative?
// Review: what is the maximum Warfarin dose and is it validated?

// 4. Audit trail completeness
prescription.Approve(approverId, utcNow);
// Review: does this record who approved it and when? Is it stored immutably?

// 5. Concurrency — two pharmacists, one prescription
// Review: does the prescription entity use optimistic concurrency?
// If not: two pharmacists can both read Draft, both approve, no conflict detected

// For clinical code specifically — test the boundary conditions explicitly:
// "INR of exactly 2.0 should be in range"
// "INR of 1.99 should be out of range"
// "INR of 3.0 should be in range"
// "INR of 3.01 should be out of range"

Slow code reviews are a significant source of delivery drag.
A PR sitting for 2 days has a blocked engineer, a context-switch cost,
and a merge conflict when it finally lands.

Targets that work for most teams:
  → First response within 4 business hours (not necessarily a full review)
  → Full review within 1 business day for standard PRs
  → Same-day review for hotfixes / urgent items

To hit these targets without sacrificing quality:
  → First pass: architecture and design concerns only (10 minutes)
    If architecture is wrong — return immediately. Don't review line by line.
  → Second pass: correctness, tests, performance (20 minutes)
  → Third pass: style (5 minutes — this should mostly be automated)

Batch your review time:
  Set aside one or two dedicated review slots per day (e.g., 9am and 3pm)
  rather than context-switching throughout the day.
  PRs don't need to be reviewed the instant they arrive.

Approve with nits:
  If a PR is correct and well-designed, approve it with minor comments
  rather than blocking merge for cosmetic changes.
  "LGTM — a couple of nits below, feel free to address or not."
  The engineer can fix minor things in the next commit.
  Don't block good work on perfect style.