Code Review Excellence: The Craft That Most Engineers Never Learn

Every engineer learns to write code. Almost no one is taught to review it.

This matters because at most companies above 30 engineers, code review is where the majority of software quality decisions are made. More bugs are caught in review than in testing. More architectural debt is introduced through approved PRs than through any other mechanism. The quality of code review directly determines the quality of the system — and the growth rate of the engineers on the team.

Despite this, most code review is learned by osmosis: junior engineers mimic what senior engineers do, senior engineers mimic what they saw earlier in their careers, and the anti-patterns propagate through generations. Teams develop cargo-cult review practices — adding comments because commenting is what reviewers do, not because the comments improve the code.

The two failure modes that are most expensive:

LGTM culture: Reviews that rubber-stamp changes to maintain social harmony or avoid conflict. The cost is not felt immediately — it accumulates in the form of bugs that were not caught, abstractions that were not challenged, tests that were not written. By the time the cost becomes visible, it is impossible to trace it to the reviews that created it.

Nitpick spiral: Reviews that spend 80% of attention on formatting, naming, and style while missing correctness issues, missing edge cases, or missing the fact that the design is wrong. This is the most common anti-pattern among engineers who are technically strong but not yet thinking about review as a leverage activity.

The mental model that fixes both: code review is a teaching opportunity disguised as a quality gate. The goal is not to block bad code — it is to make the author a better engineer and the codebase a healthier system.

The quality of your review comments determines whether authors fix issues or defend them. The same technical insight can be written two ways: one that creates collaboration, one that creates defensiveness.

The four-part comment structure that works:

(1) Observation: What you see. Not "this is wrong" — "this function modifies the database in a loop."

(2) Problem: Why it matters. "At 10,000 records, this will make 10,000 individual database writes, each incurring network round-trip latency."

(3) Suggestion: A concrete alternative. "Consider batching writes using INSERT INTO ... VALUES (...), (...), (...) — most ORMs support batch insert natively."

(4) Label (optional but powerful): Signal whether this is blocking or a suggestion. [blocking], [suggestion], [nit], [question] prefixes remove ambiguity about whether the author needs to address the comment before merging.

Examples:

❌ "This is slow." (Observation only, no problem, no suggestion, no label — creates defensiveness)

❌ "You should use a batch insert here." (Suggestion without observation or problem — author doesn't understand why)

✅ "[blocking] This loop makes N database writes sequentially. At production scale (~50K records/day), this will take ~25 seconds per job and risk connection pool exhaustion. Use Model.insert_all(records) instead — it batches into a single query."

On asking questions vs. making statements: When you are uncertain whether something is a problem, frame it as a question: "Is there a reason this is not using the connection pool? I'd expect this to create a new connection per request." This invites explanation before requiring change. Sometimes the author knows something you do not.

On complimenting: If the author did something well — a clean abstraction, a well-named function, a particularly thoughtful error message — say so. Brief, specific compliments build trust that makes critical comments land better. "Nice use of the null object pattern here — this makes the nil check explicit."

The most important reader of the code you are reviewing is not the author. It is the engineer who will debug this code at 2am six months from now, having never seen it before.

This reframe changes what you look for:

Names that only make sense with context. The author knows what tmp, data, or result refers to because they just wrote the code. The future maintainer does not. If a variable requires reading the surrounding 20 lines to understand, it needs a better name.

Comments that explain the what, not the why. Most engineers comment to explain what the code does. The future maintainer can read the code — they need to understand why it does it this way. "Sort by creation date" is useless. "Sort by creation date because the UI expects chronological order and the DB returns results in insertion order, which diverges after backfills" is valuable.

Magic numbers and strings. A function that returns early if count > 100 leaves the future maintainer wondering: what is special about 100? Is this a product requirement? A performance limit? A database constraint? Constants should be named: MAX_BATCH_SIZE = 100 with a comment explaining the source.

Implicit preconditions. If a function will crash or produce incorrect results when called with certain inputs, and the function signature does not make this clear, add documentation. "Assumes user_id is already validated" is not obvious from def process_payment(user_id, amount).

The test for whether the code passes the future maintainer check: Ask — "if I had never seen this codebase before and I needed to modify this function, what would I need to know that is not obvious from reading it?" Every answer to that question is a comment, a better name, or a extracted constant waiting to be added.

Code review is a two-sided obligation. Authors have a responsibility to make PRs reviewable; reviewers have a responsibility to review promptly.

PR size: The empirical data on code review effectiveness (from studies at Microsoft Research and SmartBear) shows a strong inverse relationship between PR size and review quality. Reviewers can effectively review ~400 lines of diff; above 400 lines, defect detection rate drops significantly. The target is: one PR, one conceptual change, ~200-400 lines of diff (excluding generated code and tests).

Large PRs are often a symptom of not decomposing the work before starting. The correct response to a 1,500-line PR is not to review it anyway — it is to send it back with a request to split it and a suggestion for how to do so.

Turnaround time: Code review latency is one of the highest-leverage variables in engineering velocity. At Google, the internal target is a first response within one business day. PRs that wait 3+ days for a first review kill momentum, cause authors to context-switch to other work, and create merge conflicts. If you cannot review a PR within one business day, acknowledge it and say when you can.

The reviewer's obligation for complex PRs: Read the PR description before looking at the diff. If there is no description, ask for one. A reviewer who reads the diff without understanding the intent will leave comments that miss the point. The description should answer: what does this change do, why is it needed, and what should the reviewer focus on?