Code review — what to look for: Question bank

The mnemonic for review order — CD STeReo PerSon — is your compass throughout. To avoid the two S's clashing, read it as C-D-STe-Re-o-Per-Son:

Correctness
Design
Security (the first S, inside "STe")
Tests
Readability
Performance
Style (the last S, in "PerSon")

So the leading S is Security (high priority, near the top) and the trailing S is Style (lowest priority, at the very bottom) — their positions in the word tell you which is which.

Definition The five symbols used on this page

Two formulas from the parent recur below. Every symbol they use, defined once here:

$n$ = the number of latent bugs hiding in one code change.
$p$ = the probability ( $0 \leq p \leq 1$ ) that the review catches any one bug.
$c_{0}$ = the base cost of fixing a bug at the review stage (the cheapest stage).
$k$ = the cost multiplier per stage ( $k > 1$ , often $k \approx 10$ ): each stage later, fixing costs $k$ times more.
$s$ = the stage number where a bug is caught: $s = 0$ at review, $s = 1$ in QA, $s = 2$ in production.
$R$ = the reviewer's time cost spent examining the change.

Where $c_{0} k^{s}$ comes from: cost starts at $c_{0}$ (stage $s = 0$ ) and multiplies by $k$ each stage, so after $s$ stages it is $c_{0} \cdot k \cdot k \dots k = c_{0} k^{s}$ . At production ( $s = 2$ ) that is $c_{0} k^{2} \approx 100 c_{0}$ .

Where $R < n p c_{0} (k^{2} - 1)$ comes from: with review, $n p$ bugs are caught cheaply at $c_{0}$ and $n (1 - p)$ escape to production at $c_{0} k^{2}$ , plus the time $R$ . Without review, all $n$ bugs reach production at $c_{0} k^{2}$ . Subtract: review saves $n p c_{0} k^{2} - n p c_{0} = n p c_{0} (k^{2} - 1)$ (the escaped bugs cost the same either way, so they cancel — that is where the $- 1$ appears, from $k^{2} - 1$ ). Review pays when that saving beats its price $R$ .

True or false — justify

A green CI pipeline proves the change is correct.

False — tests only exercise cases someone thought to write; the bug usually lives in the untested case, so green means "no known failure," not "correct."

Style problems and correctness problems deserve equal reviewer attention.

False — a linter can catch style for free, so human attention should go to correctness and design, which no tool can judge.

Code review's main purpose is catching typos and syntax errors.

False — the compiler/linter already catches those; review exists for judgement calls a machine cannot make (design fit, edge cases, security intent).

If a change works and passes review, no bug can ever reach production.

False — review catches each bug only with probability

p < 1

; expected

n (1 - p)

bugs still escape. Review lowers risk, it doesn't zero it.

Reviewing a 2000-line PR carefully just takes more time but is equally effective.

False — reviewer defect-detection collapses past roughly 200–400 lines; you don't just go slower, you miss more, so the answer is to split the PR.

The author is the best person to review their own change because they understand it fully.

False — understanding it fully is the problem: they see the code they meant to write, so they're blind to their own assumptions. Review needs fresh eyes.

"This belongs in utils/dates.py" is a correctness comment.

False — it's a design comment; the code works today, but wrong placement causes duplication and drift, which is a maintainability cost, not a bug.

Performance should be optimized everywhere in every review.

False — only where it matters (hot loops, big-

O

regressions); premature optimization elsewhere adds complexity for no measurable gain. See Big-O notation.

A reviewer's time

R

is always worth spending on any change.

False — it pays only when

R < n p c_{0} (k^{2} - 1)

; for a trivial change with almost no latent bugs (

n \approx 0

), the inequality can fail.

Spot the error

return items[len(items) - n:] to get the last n items — what breaks?

When n > len(items), len(items) - n is negative, so the slice starts from the end and silently returns the wrong items instead of erroring — a hidden correctness bug.

query = f"SELECT * FROM users WHERE name = '{name}'" — what's wrong?

name is user input spliced into SQL, so an input like '; DROP TABLE users; -- executes as code — classic SQL injection. Use a parameterized query. See SQL injection and input validation.

A reviewer writes: "You always forget edge cases." — what's the flaw?

It attacks the person, not the change, making the author defensive rather than motivated. Comment on the code: "this loop throws on an empty list."

A PR adds parse_date() to UserController and it works perfectly. What should the reviewer still say?

Flag the design: date parsing isn't a user concern and utils/dates.py already has parse_iso(); reusing it prevents duplication even though nothing is broken.

Reviewer starts by commenting on indentation and variable spacing across the diff. What went wrong?

They spent scarce attention on the cheapest-to-fix, automatable layer (style) before checking correctness/design — the 80/20 is inverted.

A reviewer approves without reading the linked ticket, just checking the code compiles. What's missing?

You can't judge a solution without knowing the problem — the code may run flawlessly while solving the wrong thing entirely.

Why questions

Why does bug-fix cost grow roughly as

c_{0} k^{s}

(where

s

is the stage: 0=review, 1=QA, 2=production) rather than staying flat?

Later stages involve more people, more surrounding code built on the bug, and real users affected — so cost multiplies by a factor

k

per stage

s

, giving exponential growth

c_{0} k^{s}

Why read the tests before the implementation?

The tests document the intended behaviour, so they tell you what "correct" even means for this change before you judge whether the code achieves it.

Why is design reviewed before readability, even though bad naming is annoying?

Design mistakes are the hardest to reverse and can cost weeks; a bad name is a five-minute rename, so you spend scarce attention where reversal is expensive.

Why does the expected-value formula favour reviewing more when the reviewer is better (

p

↑, where

p

is the catch-probability)?

A higher

p

means more of the

n

bugs are moved from the costly production stage (

k^{2}

) down to the cheap review stage, widening the savings

n p c_{0} (k^{2} - 1)

Why prefix a comment with nit:?

It signals the comment is a preference, not a blocker, so the author knows they can merge without addressing it — separating "must fix" from "would be nice."

Why is "Forecast-then-Verify" more effective than just reading the diff top to bottom?

Predicting what the change should contain primes you to notice what's missing or surprising — gaps you'd skim past if you only followed the author's presented path.

Why does automating style with a linter improve human reviews?

It removes the tempting, easy-to-spot noise, freeing the reviewer's limited attention for the judgement-heavy layers (correctness, design) a tool cannot handle. See Technical debt.

Edge cases

last_n(items, n=0) — what should happen, and does the naive items[len(items)-n:] handle it?

n=0 should return an empty list, but items[len(items)-0:] is items[len:] which is [] — accidentally correct here, though the fixed version needs an explicit if n > 0 else [] guard.

last_n(items, n=-2) — what does the naive items[len(items)-n:] do with a negative n?

With n=-2 the slice becomes items[len(items)+2:], an index past the end, so it silently returns [] — nonsense for "last −2 items"; the if n > 0 else [] guard rejects negatives cleanly.

last_n(items, n) when n > len(items) — why is this the dangerous case, unlike n=0?

Here len(items) - n goes negative, so slicing wraps and returns a wrong non-empty result silently — no crash, no empty list, just quietly incorrect data, which is the worst failure mode.

A change adds a feature but writes zero new tests, only relying on existing ones. Is that reviewable-as-correct?

No — if new behaviour isn't exercised by any test, its failure path is unverified; request a test that hits the new behaviour and its edge cases. See Unit testing.

A PR is a pure rename with no logic change. Does the priority ladder still apply?

Yes but it collapses: correctness/design are near-trivial, so the review is mostly checking the rename is complete and consistent — a case where a fast approval is genuinely appropriate. See Refactoring.

What input is the "empty collection" trap in a loop that computes an average?

An empty list makes the count zero, so dividing by it throws or returns NaN — the degenerate size-zero case that happy-path testing almost always skips.

A concurrency change passes all tests every time you run them locally. Is it safe?

Not necessarily — race conditions are non-deterministic and may only surface under specific timing/load, so passing tests can't prove absence of the bug; reason about the shared state directly.

The change touches an authorization check. Which ladder rung fires even if correctness looks fine?

Security — a missing or weakened authz check lets the wrong user act; it beats readability and style, so flag it as blocking.

Connections

Code review — what to look for — the parent; every trap here targets a WHY from that note.
Unit testing — the "passes tests ≠ correct" trap lives here.
SQL injection and input validation — the security spot-the-error item.
Big-O notation — behind the "performance only where it matters" trap.
Refactoring — the pure-rename edge case.
Technical debt — what design comments in review prevent.
Pull requests and Git workflow — the mechanism these reviews ride on.