Code review — what to look for: Exercises — Coding deep dive

The priority ladder we keep referring to — memorise it before you start:

Level 1 — Recognition

Goal: can you name the concept when you see it?

Exercise 1.1

The mnemonic for review order is "CD STeReo PerSon". Write out the seven layers in order, from first-reviewed to last.

Recall Solution

Correctness → Design → Security → Tests → Readability → Performance → Style.

Correctness, Design, Security, Tests, Readability, Performance, Style.

Why this order? Top of the list = highest impact, hardest to reverse. A wrong design costs weeks to unwind; a wrong space costs a keystroke. You spend scarce attention where it pays most.

Exercise 1.2

For each statement, say whether it is True or False:

The compiler's job overlaps with the reviewer's job of catching typos.
A code review happens after a change is merged.
Style checks should be done by a human, not a linter.

Recall Solution

True — the compiler already catches typos/syntax, so the reviewer should not spend attention there. That's the whole point: humans review what machines can't.
False — review happens before merge into the shared codebase. Reviewing after merge defeats the purpose (the bug is already in).
False — style is the one layer we delegate to a linter/formatter. Humans reviewing spacing is wasted attention.

Level 2 — Application

Goal: apply the rule to a concrete diff.

Exercise 2.1

A reviewer sees this function. Identify the single highest-priority problem and classify it on the ladder.

def average(nums):
    return sum(nums) / len(nums)

Recall Solution

Highest-priority problem: Correctness — the edge case of an empty list. If nums == [], then len(nums) == 0 and we divide by zero → the program crashes (ZeroDivisionError).

Correctness sits at the top of the ladder, so this beats any naming or style comment you might also want to make. Fix: decide the intended behaviour for empty input, e.g. return 0 if not nums else sum(nums) / len(nums), and add a test for the empty case — the tie-in to Unit testing is: the bug lives in the case nobody tested.

Exercise 2.2

Which ladder layer does each comment belong to? (Correctness / Design / Security / Tests / Readability / Performance / Style)

"This helper duplicates utils/dates.py::parse_iso() — reuse it."
"You interpolate name straight into the SQL string."
"Rename d to elapsed_days."
"This for inside a for makes the lookup $O (n^{2})$ ; a set makes it $O (n)$ ."

Recall Solution

Design — right code, wrong place; duplication → Technical debt.
Security — user input in a query string = SQL injection.
Readability — naming, understandable in 6 months.
Performance — a Big-O regression in a hot path.

If you had to drop comments due to time, keep them in ladder order: 2 (Security) first, then 1 (Design), then 4, then 3.

Level 3 — Analysis

Goal: reason about trade-offs and the "why," not just spot the flaw.

Exercise 3.1 (numeric)

Use the cost ladder $cost_{fix} \approx c_{0} \cdot k^{s}$ where $s$ is the stage (0 = review, 1 = QA, 2 = production) and $k = 10$ . A bug costs $5 to fix at review ( $c_{0} = 5$ ).

(a) What does the same bug cost if it escapes to production?
(b) How many times more expensive is that than fixing it at review?

Recall Solution

(a) Production is stage $s = 2$ : $cost = c_{0} \cdot k^{2} = 5 \cdot 1 0^{2} = 5 \cdot 100 = $500.$ (b) Ratio $= \frac{c _{0} k ^{2}}{c _{0} k ^{0}} = k^{2} = 100$ times more expensive.

Why this matters: the cost is exponential in the stage, so moving a bug even one rung down the ladder (review instead of QA) divides its cost by $k = 10$ . That is the entire economic case for reviewing.

Exercise 3.2 (numeric)

A change contains $n = 4$ latent bugs. Your review catches each with probability $p = 0.5$ . Using $c_{0} = 5$ , $k = 10$ , the "review is worth it" rule from the parent note is $R < n p c_{0} (k^{2} - 1) .$ What is the largest reviewer-time cost $R$ (in dollars) for which reviewing still pays off?

Recall Solution

Plug in the numbers: $R < n p c_{0} (k^{2} - 1) = 4 \cdot 0.5 \cdot 5 \cdot (100 - 1) = 10 \cdot 99 = $990.$ So as long as the reviewer's time costs less than $990, reviewing this change is a net win.

Read the formula: each factor pushes the threshold up — more bugs ( $n$ ), a sharper reviewer ( $p$ ), pricier failures ( $k$ ) all make review more clearly worth it. Notice the break-even scales with $k^{2} - 1$ , i.e. almost the full production penalty.

Level 4 — Synthesis

Goal: run a full review, combining several layers on one diff.

Exercise 4.1

Review this pull request. List every issue you find, tag each with its ladder layer, and give the order in which you'd address them.

def get_user_row(name):
    query = f"SELECT * FROM users WHERE name = '{name}'"
    rows = db.execute(query)
    return rows[0]                      # return the matching user

Recall Solution

Four issues, sorted top-down by the ladder:

Security — name is interpolated into the SQL string → SQL injection. An input like '; DROP TABLE users; -- is catastrophic. Fix: parameterize: db.execute("SELECT * FROM users WHERE name = %s", (name,)).
Correctness — rows[0] assumes at least one row. If no user matches, rows is empty and this throws IndexError. Fix: return rows[0] if rows else None and decide the contract.
Tests — there is no test for the "user not found" path or for a malicious name. Fix: request tests covering both (Unit testing).
Readability — SELECT * fetches unknown columns; naming the columns is clearer and cheaper to maintain. (Minor — prefix nit:.)

Order to raise them: Security → Correctness → Tests → Readability. Everything above is a blocking comment except the last, which is a nit:. You'd raise this via a pull request comment thread, commenting on the code, never the person.

Level 5 — Mastery

Goal: reason at the boundary — degenerate inputs, break-even, and process design.

Exercise 5.1 (edge/limit analysis)

Return to last_n from the parent note:

def last_n(items, n):
    return items[len(items) - n:]

with items = [10, 20, 30] (so len(items) = 3). Compute what the function returns for every case of n: (a) n = 2, (b) n = 0, (c) n = 5 (bigger than the list), (d) n = 3. Which cases are wrong, and why?

Recall Solution

The slice is items[3 - n:]. Python slicing rule: a negative start index counts from the end.

`n`	start `= 3 - n`	slice	returns	correct?
(a) 2	`1`	`items[1:]`	`[20, 30]`	✅ last 2
(b) 0	`3`	`items[3:]`	`[]`	✅ last 0 = empty
(c) 5	`-2`	`items[-2:]`	`[20, 30]`	❌ wanted all 3, got 2
(d) 3	`0`	`items[0:]`	`[10, 20, 30]`	✅ whole list

The bug (case c): when n > len(items), 3 - n goes negative, and a negative start silently wraps to the end of the list instead of clamping to the start. No exception is raised — it returns the wrong items quietly, the worst kind of bug. Fix (from parent): return items[-n:] if n > 0 else []. With n = 5, items[-5:] clamps to the whole list [10, 20, 30]. ✅

Exercise 5.2 (process, numeric)

Studies say reviewer defect-detection collapses past ~400 lines. A teammate opens a 1600-line PR. If you split it into equal PRs of ≤ 400 lines each, what is the minimum number of PRs?

Recall Solution

$PRs = ⌈ \frac{1600}{400} ⌉ = ⌈ 4 ⌉ = 4.$ Minimum 4 pull requests. Why: below the ~400-line threshold each diff still gets real scrutiny; a single 1600-line review gets rubber-stamped, so the split is what makes review actually happen. This is Technical debt-prevention at the process level.

Exercise 5.3 (break-even reasoning)

Using the rule $R < n p c_{0} (k^{2} - 1)$ with $c_{0} = 5$ and $k = 10$ : a trivial one-line typo-fix PR has essentially $n = 0$ latent bugs. What does the rule say the maximum worthwhile reviewer time $R$ is, and what's the practical lesson?

Recall Solution

With $n = 0$ : $R < 0 \cdot p \cdot 5 \cdot (100 - 1) = 0.$ The threshold is $0 — the rule says no positive reviewer time is justified. Lesson: the economic case for review scales with how many bugs a change can plausibly hide. Trivial, mechanical changes carry almost no risk, so heavyweight review of them is waste — reserve deep review for changes where $n$ (and the blast radius) is large. Review effort should track risk.

Recall One-line summary to lock it in

Review top-down (Security/Correctness/Design beat Style), read the cost ladder as exponential ( $k^{s}$ ), test the input that makes an index go negative or a list go empty, split PRs past ~400 lines, and spend review effort in proportion to risk ( $n$ ).

Connections

Unit testing — every "add a test for the untested case" answer above.
SQL injection and input validation — Exercises 2.2, 4.1.
Big-O notation — Exercise 2.2 performance tag.
Pull requests and Git workflow — the mechanism reviews and splits ride on.
Technical debt & Refactoring — design/duplication comments, PR splitting.