AR8: Engineering standards and code review
Code review
🎯You'll be able to
Explain what code review is for, review against a checklist, and keep pull requests small enough to review well.
🏠The everyday picture
Before a surgeon operates, a checklist is read aloud: right patient, right side, allergies checked. It takes a minute and catches the mistakes skilled people still make when busy.
📘What it really is
- Review is for: catching problems that would hurt customers, money or data; sharing knowledge; and keeping the codebase consistent.
- Not for: style a tool can check, or rewriting code the way the reviewer would have.
-
A checklist groups what to look for (correctness, security, tests, readability, operability), each item pointing to a rule or marked as judgement.
-
Small pull requests get better reviews; long ones get skimmed. Aim for changes a reviewer can read carefully in one sitting.
- Order of attention: first what would hurt (money, data, security), then tests, then design, then smaller points.
📝A worked example
Input: PR 41 in reviews/pr-41.diff, adding refunds.
Steps:
- Security: a token written in the code (line 5).
- Correctness: the refund call isn't awaited (line 10); the empty catch hides failures and the ledger records a refund that may not have happened (lines 11 and 12); nothing stops a refund larger than the payment.
- Tests: only the happy path is tested.
- Smaller points:
console.loginstead of the logger (line 8).
Expected output: findings in order of harm, ready to write up (Lesson 4).
🌍Where it's used
- Most product teams require at least one review before merging.
- Banks and healthcare companies require reviews for audit, often two for sensitive code.
- Open-source maintainers review contributions from people they've never met.
📝Case scenario
A team reviewed 2,000-line pull requests with a quick "looks good". A rounding bug in an invoice change slipped through and overcharged customers for a month. They limited pull requests to small, focused changes with a checklist, and the next similar bug was caught in review.
🖱️Try it
Review a small change from any open-source project's recent pull requests using the five checklist groups. Success: at least one finding or one specific piece of praise in three groups.
⚠️Common mistakes
- Rubber-stamping. Sign: approval in seconds on a large change. Fix: small changes and a checklist.
- Reviewing style. Sign: comments about spacing. Fix: let the formatter and linter do it.
- Starting with nits. Sign: ten small comments and the money bug missed. Fix: look for harm first.
🛠️For your project
Project 3's test 7 checks docs/review-checklist.md: at least ten items in at least four groups, each pointing to a rule or judgement.
✅Quick check
What is code review mainly for?
Show the answer
Catching problems that would hurt customers, money or data, sharing knowledge and keeping the code consistent.
Why do small pull requests get better reviews?
Show the answer
A reviewer can read them carefully; long ones get skimmed.