Code review anti-patterns and the metrics worth measuring Intermediate
Estimated reading time: 3 min read
This page answers: what broken review looks like, what to measure to see it without making it worse, and when it’s fine to break the rules.
Anti-patterns
Section titled “Anti-patterns”Loading diagram…
| Anti-pattern | Sign | Fix |
|---|---|---|
| Rubber stamp | An 800-line PR approved after 3 minutes, with no comments | Reviewers are as responsible as authors (Google: reviewers have “ownership and responsibility”); if you can’t read carefully, say “I only looked at part X” |
| Gatekeeping | One person must approve every PR; blocking because “that’s not how I’d do it” | Multi-person CODEOWNERS; blocking must rest on principles or data |
| Nitpick storm | 20 nit:s burying one real issue |
Move style into the linter; cap nits; put blocking comments first |
| Reviewing a 2,000-line PR | “LGTM” out of surrender | Ask for a split (stacking, vertical slices); at minimum review the design and hand it back |
| Rewriting the PR in comments | The reviewer pastes 80 lines of “their” code | Need a different design: hop on a call, then the author writes it. For small code, use “suggested change” |
| Bike-shedding | The longest thread in the PR is about a local variable’s name | Label it nitpick, let the author decide |
| Vague conditional approval | “LGTM but take another look at the retry part” | Either make it blocking, or say clearly that no re-review is needed |
| “Clean up in the next PR” | A TODO with no ticket, a third flag on a function | Clean up new complexity now; old issues get a ticket and an assignee |
Metrics: what to measure healthily
Section titled “Metrics: what to measure healthily”| Metric | Healthy when used to | Toxic when |
|---|---|---|
| Time to first response | Spot forgotten PRs; target under 1 business day | — |
| Time from PR open to merge (p50, p90) | See the bottlenecks of the whole process | Compared between individuals |
| PR size (p50, p90) | Track the trend of PRs bloating | A hard cap makes people split PRs meaninglessly |
| Number of review rounds | A PR bouncing back often → the description or design wasn’t clear up front | Ranking “who gets the most comments” |
| Comments per reviewer | — | As a KPI: breeds nitpick storms |
| PRs per person | — | As a KPI: breeds meaningless tiny PRs, fake splitting |
| Rate of “approve without comments” | A rubber-stamp signal if it spikes on large PRs | Forcing comments for the sake of it |
Principle: measure the process (latency, size, rounds), not people. Metrics tied to individual performance get optimized in ways that ruin review.
Requiring 2 approvals: the gains
-
Suits code for money, access control, production migrations.
-
Two perspectives, less risk of one person skimming.
-
Double the waiting time; easily leads to “the second person approves because the first already did”.
-
For most services, one suitable reviewer + good tests beats two superficial reviewers.
Requiring 2 approvals: the costs
When to apply / exceptions
Section titled “When to apply / exceptions”| Rule | Reasonable exception |
|---|---|
| Small, single-purpose PRs | Deleting whole files, generated code, refactors by trusted tools (Google considers these acceptable), lockfiles |
| Clean up complexity before merging | Hotfix during an incident: merge the minimal fix, open a cleanup ticket right after |
| Respond within 1 business day | On incident duty or on leave: the system must auto-assign another reviewer |
| Review before merging | Full-time pair/mob programming is already continuous review; still worth a PR so CI runs and history is kept |
| Comments per Conventional Comments | A small team with its own working nit:/blocking: convention: keep what works |
Next: Common mistakes.