Code Review

Line by Line — Building a Code Review Culture That Works
● draft ◐ in review ✓ approved
review_notes.md

Line by Line
building a code review culture that works

A good review isn't a gate the code passes through — it's the one place a second brain reliably looks at the change before it becomes everyone's problem.

7 MIN READ ENGINEERING PRACTICE

Every team says it does code review. Far fewer teams get value from it. The difference usually isn't the tooling — it's whether review is treated as a real step in building the thing, or as a formality that happens to a pull request on its way to merged.

The mechanics are the same everywhere: someone opens a diff, someone else reads it, comments happen, changes happen, eventually it merges. What varies enormously is what the reader is actually looking for, and what the team has silently agreed a review is for.

01What a diff is actually asking

A pull request is a question in disguise: "does this change do what it claims, without breaking anything it doesn't mention?" A reviewer who only checks style is answering a much smaller question than the one being asked.

auth/session.py+3 −1
12 def refresh_token(user):
return issue_token(user, ttl=3600)
+ if user.is_suspended:
+ raise SuspendedAccountError()
+ return issue_token(user, ttl=3600)
RV
reviewer · on line 14
Is there a test covering a suspended user hitting refresh? This is the actual security-relevant part of the diff — the token issuance line didn't change.

That comment isn't about naming or formatting. It's the reviewer doing the one thing a second pair of eyes is genuinely good for: noticing what the diff doesn't cover, which the author — three hours deep in the file — is structurally unlikely to see in their own work.

02Small, frequent, and honest about size

Review quality degrades hard as diff size grows. A 40-line change gets read; a 900-line change gets skimmed and rubber-stamped, because no reviewer can hold that much context at once. The single biggest lever a team has over review quality isn't a checklist — it's keeping changes small enough to actually read.

  • Split by concern, not by convenience. A refactor and a behavior change in the same diff force the reviewer to untangle which lines matter for correctness.
  • Land infrastructure before the feature that needs it. A boring "add the new endpoint, unused" PR reviews faster than one PR that adds the endpoint and wires it up at once.
  • If a PR needs a summary paragraph to be reviewable, it's a signal, not just a courtesy. Long summaries often mean the diff should have been two diffs.

03Where review culture breaks down

01

Nitpick-only reviews

Comments that only ever touch naming, spacing, and import order train authors to expect review as friction rather than insight — and let real issues through because the reviewer's attention went to the cheap stuff.

02

Approving to unblock, not because it's right

Social pressure to not be the bottleneck leads to approvals that are really just "I didn't find time to read this." The label says approved; the review never happened.

03

Comments phrased as verdicts, not questions

"This is wrong" forces a defensive reply. "What happens here if the list is empty?" invites the author to check — and is right just as often, without the friction.

04

No shared bar for what blocks a merge

Without agreement on what's a blocking comment versus a suggestion, every review becomes a negotiation about the comment's own severity before anyone discusses the code.

Review is the last cheap moment to change something. Everything after merge costs more.

04A workable default

Teams that get consistent value from review tend to converge on a similar shape: comments are labeled by severity (blocking vs. nit vs. question), the author responds to every thread rather than silently pushing a fix, and reviewers are expected to explain why, not just flag what. None of that requires new tooling — it requires the team agreeing, once, on what a review is supposed to accomplish, so every diff isn't relitigating the question from scratch.

line_by_line.md the second reader is the point
Share this post