Growth Notes

Code Review

Tags: web-dev concept

Code Review

Date: 2026-08-17

Reading someone else’s change before it merges. Its real value is spreading understanding of the codebase rather than catching bugs — and the single strongest predictor of a useful review is how small the change is.

Code review is examination of a proposed change by someone other than its author, before it merges.

What it’s actually for

The stated purpose is finding defects. The larger benefits are the ones nobody puts in the process document:

  • Spreading knowledge. After review, two people understand that code. This is the main defence against a codebase where one person owns each area
  • Consistency. Conventions propagate through review far more effectively than through documentation
  • Design feedback while it’s cheap, before the approach is fully built
  • A record of reasoning, attached to the change permanently
  • Catching defects — real, and generally the smallest of these

Size is the dominant variable

Review quality collapses as changes grow, and the collapse is sharp:

< 200 lines     read properly, real
                comments
200–400         skimmed
400–1,000       "looks good to me"
> 1,000         approved without reading

A 1,000-line pull request receives less scrutiny than a 100-line one, not more. Reviewers do not scale their attention to the size of the request — they run out of it.

So the highest-leverage thing an author can do is split the change. Everything else in this note is secondary to that.

What to look for, in order

1  CORRECTNESS      does it do what it
                    claims? edge cases?
                    error paths?
2  SECURITY         injection, authorisation,
                    secrets, input handling
                    — Common Vulnerabilities
3  DESIGN           is this the right shape?
                    ← raise EARLY or not at all
4  TESTS            do they test behaviour,
                    and would they fail?
5  READABILITY      will this make sense
                    later?
6  CONSISTENCY      does it match how we do
                    things here?

See: Common Vulnerabilities

Design feedback has a short window. Raising “this should be a different abstraction” on a finished, tested implementation is expensive and demoralising — that conversation belongs before the work, not at review.

What to leave alone

FORMATTING          automate it — Formatting
STYLE PREFERENCES   automate it or drop it
                    — Linting
"I'd have done it   not a defect
 differently"
BIKESHEDDING        naming debates on
                    internal variables

See: Formatting · Linting

Anything a tool can decide should be decided by a tool. Review time spent on formatting is review time not spent on correctness, and it makes reviews feel adversarial for no benefit.

Writing comments

BAD                      GOOD
"This is wrong"          "This will throw if
                          `items` is empty —
                          line 40 assumes at
                          least one"

"Why did you do it       "What's the reason
 this way?"               for the manual loop
                          here rather than
                          `map`? Might be
                          something I'm missing"

"Use const"              (a linter should
                          have caught this)

Distinguish blocking from non-blocking. A convention that removes most review friction:

blocking:    must change before merge
suggestion:  take it or leave it
nit:         trivial, non-blocking
question:    genuinely asking
praise:      say when something is good

Labelling a comment as non-blocking is the single cheapest improvement to review tone, because the default reading of any comment is “you must change this”.

For the author

  • Review your own diff first. It catches the debug logging and the accidental file, and it’s the cheapest possible pass
  • Write the description. What changed, why, how you tested it, what you’re unsure about
  • Flag your own uncertainty. “I wasn’t sure about the caching here” directs attention where it’s most useful
  • Keep it small. Repeated because it’s the whole game
  • Separate refactors from behaviour changes, in different commits or different pull requests — Commit Hygiene

Speed matters more than it looks

A pull request waiting two days blocks the author, ages against main, and accumulates conflicts — Branching Strategies.

Review within hours, not days. A team that reviews quickly can keep branches short, which is what makes everything else in this section work. A slow review culture forces long branches whatever the stated strategy.

Where it doesn’t work

  • As the only quality gate. Humans miss things reliably. Tests, types and linting catch what review can’t — Continuous Integration
  • Rubber-stamping. An approval that took ninety seconds on a 600-line change is a ritual, not a review
  • As a gatekeeping mechanism, where one person’s approval is required for everything. It creates a bottleneck and a single point of knowledge
  • Without a decision rule. Two reviewers disagreeing needs a named tiebreaker, or the change stalls

Graph View

Backlinks