Code Review, in depth: running review as a team system with small changes, fast turnaround, prepared authors, labelled comments, ownership rules and metrics that do not lie

What review is for

Review does several jobs at once, and a team should agree which ones it values. It spreads knowledge, so more than one person understands each part of the system. It keeps the design consistent, so the codebase does not become a collection of individual styles. It catches defects, though tests and static analysis catch many classes of defect more reliably than a human reading text. And it raises maintainability: names, structure, comments and tests that the next person will need.

What review is not: a substitute for testing, a gate where seniors prove their standards, or a place to redesign a feature that should have been discussed before the code was written. Large design questions belong in a design document, reviewed as described in how to review a design doc. Google's published engineering practices state the governing principle well: reviewers should favor approving a change once it definitely improves the overall code health of the system, even if it is not perfect. Perfection is not the bar; improvement is.

The review loop and where time goes

A change passes through the author's preparation, automated checks, reviewer assignment, a first human response, some number of revision rounds, approval and merge. Each handoff is a queue. A reviewer may read a 200-line change in fifteen minutes, but if the request sits for a day, then the author takes half a day to respond to comments, and each of two more rounds waits another half day, elapsed time is three days for under an hour of attention.

That arithmetic tells you where to improve. Making reviewers read faster barely helps. Reducing waiting between handoffs, reducing the number of rounds and keeping changes small enough to finish in one sitting all help a lot. The rest of this guide is organised around those three levers.

Size: make changes reviewable

Reviewer attention degrades as a change grows. Past a few hundred changed lines, reviewers start skimming, and the approval of a large change often means only that nothing alarming was noticed. Small changes get faster responses, fewer rounds and more careful reading. Many teams set a soft limit, for example around 400 changed lines excluding generated files, with explicit exceptions for mechanical changes.

Splitting takes practice. Separate refactoring from behaviour change: first a change that moves code without changing what it does, which reviews quickly, then the behaviour change on top, which is now small. Land infrastructure before its callers, such as a new interface and its tests before the code that uses it. Hide incomplete features behind flags, as described in feature flags, so that partial work can merge safely. Stack dependent changes, each reviewable on its own, rather than one branch that grows for two weeks. Keep purely mechanical changes, such as renames or formatter runs, in their own pull request and say so in the title.

What the author owes the reviewer

A good description cuts a round of questions. It should say why the change exists, what it does, how it was tested and what could go wrong. A template in the repository makes this the default rather than a courtesy:

<!-- .github/pull_request_template.md -->
## Why
Link the issue or describe the problem in two or three sentences.

## What changed
The approach, and anything a reviewer might find surprising.

## How it was tested
Commands run, cases covered, screenshots for UI changes.

## Risk and rollout
What breaks if this is wrong; flag, migration or rollback plan.

## Review guidance
Where to start reading; which files are mechanical.

Before requesting review, the author reviews their own diff in the same tool the reviewer will use. This catches debugging leftovers, unrelated edits and missing tests, and it is where authors notice that their change should have been two changes. Authors can also leave comments on their own pull request pointing at the important parts, for example noting that a file is a pure move. Finally, the author should make sure automated checks pass before asking a human, because a human should never spend attention on something a machine would have flagged.

Turnaround: agree a response norm

Google's guidance sets one business day as the maximum time to respond to a review request, and it also advises not to interrupt focused work to do a review but to wait for a natural break. Those two rules together are a good default. A response does not have to be a full review: a quick note saying you will look this afternoon, or that someone else should review, unblocks the author's planning.

Make review a scheduled activity rather than an interruption. Many teams use two review slots per day, for example after stand-up and after lunch. Rotate a reviewer-of-the-day for unowned changes so requests do not sit in a shared queue that everyone assumes someone else is watching. Waiting on review is also a developer-experience problem, discussed with CI wait time and merge queues in developer experience architecture.

What reviewers look at, in priority order

Spend attention in proportion to cost of being wrong. First, correctness and design: does the change do what the description says, does it handle failure and concurrency, and does it fit the existing architecture. Second, tests: do they test behaviour rather than implementation, and would they fail if the change were wrong. Third, operability: logs, metrics, configuration, migrations and how it will be rolled back. Fourth, readability: names, structure and comments that explain why. Last, style, which should be entirely automated with formatters and linters so that humans never comment on it. A reviewer who has found three style issues and no design questions has usually reviewed the wrong layer.

Comments that get acted on

Most friction in review comes from ambiguity about whether a comment must be addressed. Labels remove it. The Conventional Comments convention prefixes each comment with a label such as praise, nitpick, suggestion, issue, question or thought, optionally decorated as blocking or non-blocking. Google's guidance uses a Nit: prefix for points of polish the author may ignore. Any consistent scheme works; having one is what matters.

Write about the code, not the person, explain the reason, and where possible propose concrete code. Ask questions when you do not understand rather than asserting; sometimes the author knows something you do not. And mark the few things that truly block, so that everything else can be handled in a follow-up.

Disagreement and what approval means

When author and reviewer disagree, first move from text to a short call, because long comment threads amplify tone. If they still disagree, the team needs a tie-breaker that is decided in advance: the code owner, the tech lead, or a written team convention. Style disagreements end with adding a rule to the linter configuration, so they never recur.

Define approval explicitly. A common convention is that approve means I would be comfortable being paged for this, and approve with comments means merge after addressing the non-blocking comments at your judgement, without another round. Requesting changes should be reserved for blocking issues. If new commits arrive after approval, branch rules can dismiss stale approvals so that the approved code is the merged code.

Ownership and rules as configuration

Who must review what is a policy, and it belongs in version control. A CODEOWNERS file maps paths to owning teams; when several patterns match a file, the last matching pattern in the file wins.

# .github/CODEOWNERS  (last matching pattern takes precedence)
*                       @acme/platform-reviewers
/services/billing/      @acme/billing
/services/billing/db/   @acme/billing @acme/dba
*.tf                    @acme/infra
/.github/               @acme/devex

Combine it with branch protection or rulesets on the default branch: require pull requests, require at least one approval, require review from code owners, dismiss stale approvals on new commits, require status checks to pass and require branches to be up to date or use a merge queue. Keep owner groups as teams rather than individuals, so vacations do not block merges, and review the CODEOWNERS file itself like code.

Worked example: splitting a large change

An engineer opens a 1,100-line pull request that adds per-customer rate limiting to an API. It touches the request middleware, adds a Redis client wrapper, changes configuration loading, reformats two files and adds tests. After two days there is one comment: looks good overall, a few questions. Nobody has really reviewed it.

Split, it becomes four pull requests. First, the formatter run on the two files, labelled mechanical, approved in minutes. Second, the configuration change with its tests, about 150 lines. Third, the Redis wrapper and its tests, about 300 lines, reviewed by the infrastructure owner. Fourth, the middleware change behind a flag that defaults off, about 350 lines, reviewed by the API owners, who can now focus on the actual limit logic and spot that limits reset on deploy. Each merges within a day of being opened, and the defect that the large change hid is caught in the small one.

Measuring review without gaming it

Measure the flow, not individuals. Useful measures are time to first review, time from open to merge, number of review rounds, the distribution of change sizes and how concentrated review load is across people. The GitHub CLI can export what you need:

import json, statistics, subprocess
from datetime import datetime

def ts(s): return datetime.fromisoformat(s.replace("Z", "+00:00"))

prs = json.loads(subprocess.check_output([
    "gh", "pr", "list", "--state", "merged", "--limit", "200",
    "--json", "number,author,createdAt,mergedAt,additions,deletions,reviews"]))

first_review_h, merge_h, sizes = [], [], []
for pr in prs:
    login = lambda x: (x or {}).get("login")      # deleted accounts come back as null
    others = [r for r in pr["reviews"] if login(r["author"]) != login(pr["author"])]
    if others:
        first = min(ts(r["submittedAt"]) for r in others)
        first_review_h.append((first - ts(pr["createdAt"])).total_seconds() / 3600)
    merge_h.append((ts(pr["mergedAt"]) - ts(pr["createdAt"])).total_seconds() / 3600)
    sizes.append(pr["additions"] + pr["deletions"])

if first_review_h:
    print("median hours to first review:", round(statistics.median(first_review_h), 1))
print("median hours to merge:", round(statistics.median(merge_h), 1))
print("median lines changed:", statistics.median(sizes))

Use medians and look at the slowest tenth as well, because averages hide the change that waited a week. Never turn these into targets for individuals: counting comments per review rewards nitpicking, and counting approvals per day rewards rubber-stamping. Look at them monthly as a team, pick one to improve, and change the process rather than exhorting people.

Failure modes

• Rubber-stamping. Large changes and social pressure produce approvals without reading. Fix size first, then make approval semantics explicit.
• Gatekeeping. One senior reviewer blocks on personal preference. Use labels, written conventions and a tie-breaker.
• Review as design. Fundamental objections arrive after the code is written. Move design discussion earlier with a short design note.
• Hero reviewers. One person reviews half of all changes and becomes the bottleneck. Spread ownership and pair newer reviewers with them.
• Style wars. Human comments about formatting. Automate it entirely.
• Stale approvals. Code changed after approval merges unreviewed. Dismiss stale approvals in branch rules.

What to do next

1. Write down what review is for on your team and what approve means, in one paragraph.
2. Add a pull request template and a CODEOWNERS file, both reviewed like code.
3. Configure branch rules: required approvals, code-owner review, stale-approval dismissal and required checks.
4. Automate formatting and linting so no human comments on style again.
5. Agree a one-business-day response norm and schedule two review slots per day.
6. Adopt comment labels with blocking and non-blocking decorations.
7. Run the metrics script monthly, and pick one process change at a time to improve the slowest step.