Chapter 7 · Code Review: What Makes a Review Actually Useful
Chapter 6's own estimates and Chapter 5's own stories eventually turn into code someone else has to look at before it ships. This chapter verifies three real distinctions that separate a review that catches something from one that merely feels thorough: what it actually checks for, which feedback is allowed to block a merge, and whether a rejection teaches anything at all.
Style vs. Substance: What a Checklist Actually Catches
defcalculate_invoice_total(items):
total = 0for item in items: total += item['price'] * item['quantity']
return total * 0.9defcalculate_receipt_total(items): # well-named, well-formatted - and duplicated
total = 0for item in items: total += item['price'] * item['quantity']
return total * 0.9
Verified directly — a style-only checklist approved code with a real, catalogued smell in it
A checklist checking formatting and naming alone: 0 issues found, PR approved — the code is genuinely well-formatted, with clear snake_case names and no overly long lines. A checklist checking for the specific smells Clean Code, SOLID & Refactoring catalogued: 1 issue found, PR rejected — "Duplicated calculation logic detected across two functions," the exact Extract Method finding that course's own Chapter 8 verified.
Style and substance aren't in tension — they're just different questions
Nothing about the duplicated code above would fail a linter or a formatter. A review that only automates what a linter already checks adds no real value beyond the linter itself; the human review's own job is everything a linter structurally can't see — design, correctness, and the specific smell categories a tool can't infer without understanding intent.
Blocking vs. Non-Blocking: What Gets to Delay a Merge
# each flagged item has a real, independent 90% chance of being# fixed correctly on any given attempt
FIX_SUCCESS_RATE = 0.9# ALL-BLOCKING: 1 correctness issue + 4 style nits, ALL gate merge# SELECTIVE-BLOCKING: only the 1 correctness issue gates merge
Verified directly — treating every nit as blocking cost 31% more review rounds, for the identical set of eventual fixes
Simulated over 2,000 trials: gating merge on all 5 flagged items (1 real correctness issue plus 4 style nits) took an average of 1.46 review rounds to clear. Gating merge on only the 1 correctness issue — with the 4 nits left as non-blocking, addressed in this PR or a fast-follow — took an average of 1.11 rounds. 31% more rounds, purely from treating issues that were never actually risky as merge-blocking.
Every issue still got fixed either way — this is Chapter 1's own "too much process" finding, one layer in
Non-blocking feedback isn't feedback that gets ignored — the nits still get addressed. The only thing that changes is whether merging waits on them. This is the same fixed-tax problem Chapter 1 measured directly: applying uniform gating weight to genuinely non-uniform risk produces real, measured delay with no corresponding safety benefit.
Teaching vs. Gate: What a Rejection Actually Changes
# GATE-ONLY: rejects with no explanation - the mistake rate never improves# TEACHING: explains the reasoning - each explained mistake cuts the# chance of a REPEAT by 40%
Verified directly — explaining the reasoning behind a rejection nearly halved how often the same mistake recurred
Simulated across 3,000 authors, each submitting 8 PRs: with gate-only reviews (rejected, no explanation given), the same class of mistake recurred an average of 4.80 times across those 8 PRs. With teaching reviews (the reasoning explained each time it was caught): 2.64 times — 45% fewer recurrences, from the identical starting mistake rate.
A gate protects one PR. Teaching protects every PR after it.
Both review styles caught the mistake the first time it appeared — a gate-only review isn't wrong, it's incomplete. It stops the immediate problem but leaves the underlying misunderstanding fully intact, guaranteeing it costs review time again, and again, until someone eventually explains why.
Where This Connects
This chapter's finding
What it connects to
A style-only checklist missing a real Extract Method violation
Clean Code, SOLID & Refactoring Chapter 8's own duplicated-calculation finding, reused directly as the reviewed code
31% more review rounds from over-broad blocking
Chapter 1's own fixed-tax "too much process" finding, applied specifically to code review
45% fewer recurring mistakes from explaining the reasoning
Chapter 9's own retrospective material — both are about turning a caught problem into an actual, lasting improvement
Hands-On Exercises
Exercise 1
Modify this chapter's own smell-aware checklist to also detect a second Clean Code smell: a function with more than 6 positional parameters (Chapter 4's own long-parameter-list finding). Write a sample function that triggers it, and verify the checklist correctly flags it while leaving a well-formed function alone.
Using this chapter's own blocking-vs-non-blocking simulation, lower FIX_SUCCESS_RATE from 0.9 to 0.7 (a genuinely harder or more ambiguous set of fixes). Determine the new average round counts for both policies, and explain how the gap between them changes as fixes become less reliable.
Using this chapter's own teaching-vs-gate simulation, extend the number of PRs per author from 8 to 20. Determine the new average total mistakes for both review styles, and explain whether the relative gap between gate-only and teaching grows, shrinks, or stays proportional as more PRs are observed.