The Professor
Index Course index

Module 11 · Non-technical skills

Lesson 50 — Code review

Giving and receiving feedback without breaking the team.

Published
In this lesson
  1. Objectives
  2. 1. What gets reviewed (and in which order)
  3. 2. The comment that teaches: criterion, tone, question
  4. 3. The blocker scale: the hierarchy the project defines
  5. 4. Receiving the review: the ego out of the loop
  6. 5. The PR checklist (the project's one)
  7. Self-assessment

Stack: Soft skills · Project: TicketFlow Status: Published — giving and receiving feedback without breaking the team Prerequisite: Lesson 49 — Estimating and splitting problems

Objectives

  1. Run the review with method: what gets reviewed (and in which order), what is blocking and what is a suggestion.
  2. Write review comments that teach and do not attack: the criterion, the tone and the question that beats the order.
  3. Receive the review without ego: the response to feedback, disagreement with data, and the author's PR checklist.

1. What gets reviewed (and in which order)

The effective review reads in impact order: (1) the WHAT (the PR description: is the intent clear? does the test prove the intent? — 49: explicit scope); (2) the contract (do API/DB/events change? — 35's contract tests and 48's ADR: the contract change demands its doc and 14's version); (3) security (22: user input, 21's authz, 23's secret); (4) correctness (logic, edges, 10's concurrency); (5) tests (do they prove the WHAT? does the net have holes? — 34); (6) readability last (names, duplication — what an IDE/linter already catches consumes no human time: 41's ruff/black/mypy automatic, the human on what is human). The inverted review's mistake: 40 style comments and zero about the ORM's data race (09) — order protects impact.

The project's lens (what TicketFlow ALWAYS reviews): does the service stay ignorant of HTTP/infra (24)? does the outbox go after commit (25)? does the lock protect the inventory (10)? does the error exit through the handler (26)? is the test behavioral (33)? — five questions that are the repo's DNA: the review asking them is the one maintaining the system.

2. The comment that teaches: criterion, tone, question

The review comment has three useful forms: the blocker (with technical reason and reference: "this breaks 26's contract: if the front parses type, the 409 without type breaks checkout — see test_contract_guard; I propose the central handler"), the suggestion (nit marked as such: nit: the name x → seat_refs; the reviewer classifies the weight: the author knows what to attend), and the question (the most powerful form: "what happens if two requests arrive here at once?" — the question teaching the risk WITHOUT the order; the author discovers the missing lock and learns the why, not the rule). And the tone: comment on the CODE, not the person ("this loop makes N queries" ≠ "you don't know SQL") — 47's rule (blameless) applied to the PR's text.

markdown
❌ "This is wrong, you never do queries in a loop."
✅ "This fires 1 query per reservation (09's N+1): with 40 reservations that's 83 queries.
    With prefetch_related it's 3 and the listing's performance test goes green.
    Shall we talk if the use case demands another shape?"

The grown-up comment's three pieces: the fact (the evidence), the impact (what it costs), and the path (or the question opening it). The comment without a path is judgment; the one with a path is engineering.

3. The blocker scale: the hierarchy the project defines

Not every comment weighs the same. The project's scale (agreed, not invented per review): blocker (breaks contract, security, money/inventory invariants, or the net's test), should (the N+1, the missing test in the critical zone: solved in the PR or immediate issue), nit (style/name: the author decides), question (no position: opens dialogue). The reviewer classifies and the author prioritizes: the PR with 12 nits and 1 blocker is not "12 problems". And the disagreement's review: if the author responds with data and the reviewer holds: escalation goes to the ADR (48) — the review is not decided by the voice's volume, it is decided by the decision's document (or a new ADR gets created if the discussion deserves it).

4. Receiving the review: the ego out of the loop

The review is of the CODE and for the SYSTEM: the feedback that stings ("this mixes layers") is the feedback that teaches. The author's rules: (1) answer EVERYTHING (the unanswered comment is the silent block: "done", "ok with the nit, not changing it", or the argued disagreement); (2) disagreement with data ("I keep the loop: it's 5 fixed rows and prefetch complicates; the performance test is in the PR — shall we review the number?") beats disagreement with feelings; (3) the author's PR arrives CLEAN: self-review first (your own diff read with a stranger's eyes), tests green (41), the small PR (49: the vertical piece — the 2000-line PR gets reviewed with a panicked "LGTM"); (4) fast re-review (the reviewer requesting changes answers in hours: the slow review is the tax killing 41's cadence).

And the team-of-1 case (the course): the structured self-review — §5's checklist as the virtual reviewer, the linter's bot (41) as the mechanical reviewer, and the ADR's cross-review (48: the PR's decision documented before the merge): reviewing yourself is imperfect but the METHOD approximates it.

5. The PR checklist (the project's one)

markdown
## Author's checklist (before requesting review)
- [ ] The WHAT is in the description with scope in/out (49)
- [ ] New/adjusted behavioral tests (33) and the suite green (41)
- [ ] The contract did not change (35) — or it changed WITH its version (14) and doc (48)
- [ ] The repo's 5 DNAs: layers (24) · outbox after commit (25) · lock (10)
      · error handler (26) · environment config (27)
- [ ] No secrets or PII in fixtures/logs (23)
- [ ] Self-review done: I would read this diff in an incident (47) without asking

## Reviewer's checklist (in impact order)
- [ ] The WHAT and the test proving it
- [ ] Contract/security/correctness (in that order, §1)
- [ ] The comment's classification: blocker/should/nit/question (§3)

The checklist is the repo's social contract: the review stops being personality and becomes process — what a stranger's PR and the team-of-1's self-review share.

Self-assessment

  1. In which order does the review read, and why does style go LAST? What are TicketFlow's 5 repo DNAs?
  2. Which three forms does the useful comment have, and why is the question the most powerful?
  3. The blocker/should/nit/question scale: what does it solve (the 12-nit PR) and how do disagreements escalate?
  4. Which 4 rules govern the author, and what does the self-review do in a team of 1?
  5. Why is the small PR (49) a review condition, and what does the 2000-line PR cost?

Continue with the exercises. The solutions only after trying it yourself.

^ top