Code Review Checklist

Change: _______________ Reviewer: _______ Date: _______

Order matters. Style comments are cheap and satisfying, and a review that opens with them rarely reaches the questions that prevent incidents. Work down the list.

1. Before reading the code#

• [ ] What problem does this solve? If the change does not say, ask before reviewing
• [ ] Is this the right problem to solve now?
• [ ] Is the change small enough to review properly? If not, say so and stop. A 900-line review is a rubber stamp with extra steps
• [ ] Does the description explain the approach, not just restate the diff?

2. Correctness where it is usually wrong#

• [ ] Empty input, single item, maximum size
• [ ] Null, missing, and explicitly-set-to-nothing treated distinctly where they differ
• [ ] Off-by-one at every boundary
• [ ] Concurrent execution. Can two of these run at once, and what happens?
• [ ] Time zones, daylight saving, and dates near month and year boundaries
• [ ] Numeric precision where money is involved
• [ ] Encoding assumptions on anything that came from outside

3. Failure behaviour#

• [ ] What happens when the thing it calls fails? Named, not assumed
• [ ] Timeouts set on every external call
• [ ] Retries only where a retry can succeed, and with backoff
• [ ] Retry safety. Is the operation idempotent, or can it be repeated harmfully?
• [ ] Partial failure leaves a consistent state
• [ ] Errors reach somewhere a person will see them
• [ ] Failure messages say what to do, not only what went wrong

"What happens when this fails?" is the highest-yield question in code review. Ask it about every call that leaves the process.

4. Reversibility#

• [ ] Can this be rolled back? If not, that is a design decision, is it a deliberate one?
• [ ] Database changes are backward-compatible with the currently deployed code
• [ ] Behind a flag if it is risky, and the flag has a removal plan
• [ ] Data migrations can be re-run safely

5. Security, proportionate to what it touches#

• [ ] Input from outside is validated at the boundary
• [ ] Authorisation checked where the data is fetched, not in the caller
• [ ] Queries parameterised
• [ ] Output encoded for its destination
• [ ] No secret in code, configuration file, log line or error message
• [ ] Logs do not contain personal data that will outlive its purpose

6. Tests#

• [ ] Would the tests fail if the code were wrong? Check one deliberately
• [ ] The specific bug being fixed has a test that fails without the fix
• [ ] Boundaries and failure paths tested, not only the happy path
• [ ] Tests assert on behaviour, not on internals that will change on the next refactor
• [ ] No new flaky test, if it needs a fixed wait, it is not finished

7. Change to existing behaviour#

• [ ] Anything depending on the old behaviour has been identified
• [ ] Interfaces others consume are versioned or unchanged
• [ ] Output shape unchanged, or every consumer updated
• [ ] Removed code is genuinely unused, checked, not assumed

8. Fit#

• [ ] Consistent with how this codebase already does things
• [ ] No new pattern introduced without a stated reason. One codebase with three approaches to the same problem is more expensive than any of them
• [ ] No dependency added without a reason and a look at what it pulls in
• [ ] Abstraction earns its place, one caller does not need an interface

9. Readability, last#

• [ ] Names say what the thing is
• [ ] Comments explain why, never what
• [ ] Nothing surprising left unremarked
• [ ] A stranger could follow the main path in one read

10. As the author, before requesting review#

• [ ] Read the diff yourself first
• [ ] Removed debugging output, commented-out code and unrelated changes
• [ ] Said what you are unsure about: it directs the reviewer to where they are most useful

Sign-off#

Back to Software Engineering