Code review — what to look for: Worked examples

Before any example, we build the map so no cell goes uncovered.

Mnemonic Decoding "CD STeReo PerSon" — the review priority order

This page follows the parent's priority ladder. Each chunk of the phrase is one layer, reviewed in this order (highest-impact first):

C = Correctness — does it do what it claims, on every input?
D = Design — does it fit the system, in the right place, no duplication?
S = Security — untrusted input, injection, secrets, auth.
Te = Tests — do the tests exercise the failure path, not just the happy one?
Re = Readability — will a newcomer understand it in 6 months?
Per = Performance — only where it matters (hot loops, big- $O$ regressions).
S = Style — a linter should own this; humans flag it lightly as a nit:.

Why this order? The top layers are the hardest to fix later and most expensive to get wrong; the bottom ones are cheap and automatable. You spend scarce attention top-down.

The scenario matrix

Every code-review situation lands in one of the boxes below. The columns are the priority layers from the mnemonic; the rows are the kinds of trap each layer hides. Our worked examples (E1–E8) fill every cell that actually produces a review comment.

Trap class ↓ / Layer →	Correctness	Design	Security	Tests	Perf	Style
Degenerate / empty input	E1 (empty & `n>len`)
Sign / boundary value	E1 (negative index)
Wrong place / duplication		E2
Untrusted input			E3 (SQLi)
Missing failure-path test				E4
Limiting / scaling behaviour					E5 (Big-O)
Real-world word problem	E6 (payments)
Exam-style twist (looks-safe trap)	E7 (float money)
"Just a nit" / style-only						E8 (naming)

We also fix vocabulary so nothing is used before defined:

E1 — Degenerate input × Correctness (empty list, `n` too big, negative slice)

Worked example Statement

A PR adds this helper. Review it.

def last_n(items, n):
    return items[len(items) - n:]   # "return the last n items"

Reviewer question: for which inputs does this lie about being "the last n items"?

Steps:

Trace the happy path. items=[10,20,30,40,50], n=2 → len-n = 3 → items[3:] = [40,50]. Correct. Why this step? You must confirm the intended behaviour first, so any later surprise is clearly a defect and not a misunderstanding.
Push n past the length. n=8, len=5 → len-n = -3 → items[-3:] = [30,40,50]. It returns 3 items, not the whole list of 5 — and silently. Why this step? A negative start index in Python doesn't error; it counts from the end. That silent wrap-around is exactly the "case nobody tested" the parent warned about.
Try n = 0. len-0 = len → items[5:] = []. "Last 0 items" = empty. Arguably fine, but confirm it's intended, not accidental. Why this step? Zero is the classic degenerate input; it must be a deliberate answer, not a coincidence.
Try empty items with n=2. len=0 → 0-2 = -2 → items[-2:] = []. No crash, returns []. Acceptable, but only by luck. Why this step? Empty input is the first row of our matrix — always test it explicitly.
Propose the fix that removes the silent wrap:
```
def last_n(items, n):
    if n <= 0:
        return []
    return items[-n:]
```
With n=8, len=5: items[-8:] clamps to the whole list [10,20,30,40,50] — 5 items, which is the honest answer for "last 8 of 5." Why this step? items[-n:] with n ≥ len clamps to the full list instead of wrapping; and the n<=0 guard makes zero/negative explicit.

E2 — Wrong place / duplication × Design

Steps:

Confirm correctness first. Feed it "2024-03-01" → returns a date object. No bug. Why this step? Never raise a design flag while a correctness bug is still open — priority order (CD STeReo PerSon).
Ask "does this belong here?" A UserController handles user requests; date parsing is a generic utility. Placing it here means the next dev who needs date parsing won't find it and will write a second copy. Why this step? Design bugs don't crash today; they cause duplication and drift tomorrow — see Technical debt.
Check for an existing home. The repo already has utils/dates.py with parse_iso(). This is reinvention. Why this step? "Not reinventing an existing utility" is item 2 on the parent's ladder.
Write a non-blocking, actionable comment: "This works, but date parsing isn't a user concern — utils/dates.py already has parse_iso(). Can we reuse it and drop this copy?" This is a refactor request, not a bug. Why this step? You comment on the code, not the person, and give a concrete next action.

E3 — Untrusted input × Security (SQL injection)

Steps:

Trace a benign input. name = "Alice" → SELECT * FROM users WHERE name = 'Alice'. Works. Why this step? Establish the intended behaviour before attacking it.
Trace a hostile input. name = "'; DROP TABLE users; --". The string becomes SELECT * FROM users WHERE name = ''; DROP TABLE users; --'. The ' closes the string early, ; starts a new statement, -- comments out the trailing quote. The database now runs an attacker-supplied command. Why this step? Security beats style — a data-destroying injection is catastrophic and irreversible. See SQL injection and input validation.
Fix with a parameterized query so the value is never treated as code:
```
cursor.execute("SELECT * FROM users WHERE name = %s", (name,))
```
Now name is sent as data; the same hostile string is looked up as a literal (matching nothing) instead of executed. Why this step? Separating code from data is the only reliable fix — escaping by hand is error-prone.

E4 — Missing failure-path test × Tests

Steps:

Read the test first. It only asserts the happy path (amount < balance). See Unit testing. Why this step? Tests document intended behaviour, but here they document only the easy case.
Ask "what input is NOT covered?" The failure path: amount > balance, and the boundary amount == balance, and amount = 0. Why this step? The parent's rule: the bug lives in the case nobody tested.
Probe the code with withdraw(100, 150). Suppose it returns -50. An overdraft slipped through with green CI. Why this step? Green CI is not proof of correctness — it's proof of tested cases only.

Request the missing tests, including the failure path:

assert withdraw(100, 100) == 0        # boundary
with pytest.raises(ValueError):
    withdraw(100, 150)                 # overdraft must be rejected

Why this step? A test that exercises the failure path is what makes the review meaningful.

E5 — Limiting / scaling behaviour × Performance (Big-O)

Worked example Statement

def has_dupes(items):
    for i in range(len(items)):
        for j in range(len(items)):
            if i != j and items[i] == items[j]:
                return True
    return False

Correct? Fast enough? Comment.

Steps:

Confirm correctness. [1,2,2] → finds the pair → True. [1,2,3] → False. Correct. Why this step? Never flag performance while correctness is unresolved.
Count the work. Two nested loops each of length n → about $n \times n = n^{2}$ comparisons. In Big-O notation this is $O (n^{2})$ . Why this step? Performance is only worth a comment when the growth rate is bad — here it is, on the hot path.
See the scaling. Look at the figure: doubling n roughly quadruples the work for the nested-loop version, while a set-based version grows linearly.

Figure E5. Horizontal axis = n (list length); vertical axis = worst-case number of comparisons. Two curves are drawn and also labelled directly on the plot so colour is not required to read it: the upper, steeply-bending parabola (marked "n^2 nested loops", circular markers) is the nested-loop version; the lower, nearly-flat straight line (marked "n set version", square markers) is the set version. The yellow callouts spell out the key jump in words: when n goes from 40 to 80 the nested-loop work rises from $4 0^{2} = 1600$ to $8 0^{2} = 6400$ — a 4× increase for a mere doubling of input, whereas the straight line only doubles (from 40 to 80). Alt-text: a parabola rising far faster than a straight line as n increases; at n=80 the parabola is at 6400 while the line is at 80.

Why this step? The parent said flag perf only where it matters — a quadratic on large inputs matters.

Propose the linear fix using a set (each lookup is $O (1)$ on average):

def has_dupes(items):
    seen = set()
    for x in items:
        if x in seen:
            return True
        seen.add(x)
    return False

This is $O (n)$ . Why this step? A single pass with a membership set removes the inner loop entirely.

E6 — Real-world word problem × Correctness (currency rounding)

Worked example Statement

A shopping cart splits a $10.00 bill equally among 3 friends:

share = round(1000 / 3) / 100   # cents; 1000 = $10.00
total_charged = share * 3

Does the store collect exactly $10.00? Review.

Steps:

Compute one share in cents. $1000/3 = 333.33...$ , round(...) = 333 cents = $3.33. Why this step? Money must be handled in the smallest unit (cents) as integers to reason about rounding.
Multiply back. $333 \times 3 = 999$ cents = $9.99. The store is short one cent. Why this step? Naïvely charging share × count leaks money on every non-divisible split — a real financial bug.
Flag the rounding-mode trap. Python's built-in round() uses banker's rounding (round-half-to-even), so round(2.5) == 2 and round(3.5) == 4 — halves go to the nearest even integer, not always up. For money you must choose a strategy explicitly rather than trust the default. Why this step? Two reviewers testing different half-values could see different results; financial code must state whether it rounds half-up, half-even, or (best here) avoids fractional cents entirely.

Fix: distribute the remainder so no cent is created or lost, sidestepping rounding altogether:

def split_bill(total_cents, people):
    base, rem = divmod(total_cents, people)   # base=333, rem=1
    # give the first `rem` people one extra cent
    return [base + (1 if i < rem else 0) for i in range(people)]
 
shares = split_bill(1000, 3)   # [334, 333, 333]
total_charged = sum(shares)    # 1000 cents = $10.00 exactly

Why this step? divmod splits the amount into an equal base plus an integer remainder rem; handing the rem leftover cents out one-by-one guarantees the parts sum exactly to the whole, with no rounding and no lost cent.

E7 — Exam-style twist × Correctness (the "looks safe" float trap)

Steps:

Recall why floats lie. 0.1 and 0.2 have no exact binary representation, so their stored values are slightly off. Their sum is 0.30000000000000004, not 0.3. Why this step? This is the classic "looks safe" trap — an exam favourite and a real production bug source.
Predict the assertion. 0.30000000000000004 == 0.3 is False — the test would fail (or worse, someone "fixes" it by loosening the check without understanding). Why this step? A reviewer must catch equality-on-floats even when the author is confident.
Fix: compare with a tolerance instead of ==. Either use the standard library's closeness check:
```
import math
assert math.isclose(0.1 + 0.2, 0.3, rel_tol=1e-9)
```
or, when money must be exact, switch to integer cents (see E6) or Decimal:
```
from decimal import Decimal
assert Decimal("0.1") + Decimal("0.2") == Decimal("0.3")
```
Why this step? Floating-point equality should almost always be a closeness check; Decimal gives exact base-10 arithmetic when correctness (not speed) is the priority.

E8 — Style-only × Style (the disciplined "nit")

Steps:

Confirm the higher layers are clean. Correctness / Design / Security / Tests / Performance all pass. Why this step? Only reach Style once everything above it is settled — otherwise you nitpick while a real bug hides (the parent's "nitpicking style" mistake).
Prefix the comment nit: so the author knows it's a preference, not a blocker: "nit: rename x → retry_count for readability; non-blocking." Why this step? Distinguishing blocking from non-blocking keeps the author moving and unoffended — you comment on the code, never the person.
Approve. A single naming nit should never hold up an otherwise-solid change. Why this step? Style is the layer a linter/formatter should own; humans flag it lightly and let the merge proceed.

Recall Cover-the-matrix self-test (click to reveal)

Which example handled a negative slice index? ::: E1 Which example moved code to its correct module? ::: E2 Which example separated code from data with a parameterized query? ::: E3 Which example demanded a failure-path test despite green CI? ::: E4 Which example spotted an $O (n^{2})$ growth on the hot path? ::: E5 Which example lost a cent to naïve rounding? ::: E6 Which example exposed float-equality as a trap? ::: E7 Which example modelled a purely non-blocking nit? ::: E8 What does the final S in CD STeReo PerSon stand for? ::: Style — automate with a linter, flag lightly as a nit.

Connections

Code review — what to look for — the parent: what to look for and why.
Unit testing — E4 lives here: what a good failure-path test looks like.
Big-O notation — E5's scaling argument.
SQL injection and input validation — E3's vulnerability and fix.
Refactoring — E2 and E5 both end in a refactor request.
Technical debt — why E2's design smell matters tomorrow.
Pull requests and Git workflow — the diff/PR every example rides on.