Chapter 5: Review Checklists: Functional and Non-Functional

Why this matters

Chapter 4 established the six dimensions a thorough review covers — a checklist is what turns "remember to check six things" into a reliable, repeatable practice that doesn't depend on a reviewer's memory or mood on a given day. Checklists aren't a sign of inexperience; they're a well-established tool (borrowed directly from aviation and medicine, fields where expert practitioners still rely on them specifically because human memory and attention are unreliable under real-world conditions) for making thoroughness consistent rather than aspirational.

Functional checklist: does the change actually do what it should?

[BEGINNER]

## Functional Review Checklist
- [ ] Does the code correctly implement the stated requirement?
- [ ] Are edge cases handled (empty input, null/undefined, boundary values)?
- [ ] Are error paths handled per our error handling standards (Coding
      Standards guide, Chapter 5) — no empty catch blocks, appropriate
      fail-fast vs. fail-gracefully choice?
- [ ] Do existing tests still pass, and are new tests added for new behavior?
- [ ] Does this change interact correctly with existing, unchanged code
      that depends on the modified area?

Non-functional checklist: does the change meet the codebase's broader standards?

[BEGINNER]

## Non-Functional Review Checklist
- [ ] Naming: are names clear and consistent with existing conventions
      (Coding Standards guide, Chapter 2)?
- [ ] Security: any unvalidated input, injection risk, or exposed secret
      (Coding Standards guide, Chapter 10)?
- [ ] Performance: any obvious inefficiency (N+1 queries, unbounded loops,
      unnecessary synchronous blocking) that would matter at real scale?
- [ ] Design fit: does this follow established patterns in the codebase,
      or introduce an inconsistent, one-off approach?
- [ ] Documentation: does this change require updating a README, API docs,
      or an architecture decision record?

A checklist is a floor, not a ceiling

[SENIOR INSIGHT] A checklist's purpose is ensuring baseline coverage — it prevents a reviewer from forgetting an entire dimension (Chapter 4), not replacing genuine engagement and judgment with the specific code being reviewed. A reviewer who mechanically ticks every checklist box without actually reading and reasoning about the code has satisfied the checklist's letter while missing its entire purpose — exactly the rubber-stamping anti-pattern Chapter 11 covers in depth. The checklist is a reminder of what to look for, not a substitute for looking.

Tailoring checklists to specific change types

[IN PRACTICE] A single, generic checklist applied uniformly to every PR type misses category-specific risks — a database migration has different critical failure modes (companion Database book, Chapter 13's schema-evolution safety concerns) than a frontend component change (accessibility, responsive behavior) or a public API change (backward compatibility, versioning). Mature teams often maintain a small set of specialized checklist additions triggered by the type of change:

## Additional checklist for database migrations
- [ ] Does this migration require a table lock on a large, actively-written
      table? (See Database guide, Chapter 13 on safe schema evolution.)
- [ ] Is this change backward-compatible with currently-deployed application
      code during a rolling deployment (expand-contract pattern)?
- [ ] Has this been tested against a realistic data volume, not just a
      small development dataset?

## Additional checklist for public API changes
- [ ] Is this change backward-compatible for existing API consumers?
- [ ] If not backward-compatible, is it properly versioned?
- [ ] Is the API documentation updated to reflect this change?

Common mistakes

1. Using a checklist as a mechanical box-ticking exercise without genuine engagement with the specific code — this satisfies the checklist's letter while missing its actual purpose of prompting real thought about each dimension.
2. Applying one generic checklist uniformly to every PR type, missing category-specific risks that a specialized addition (for migrations, API changes, security-sensitive code) would catch.
3. Treating a checklist as exhaustive, assuming anything not explicitly listed doesn't need consideration — a checklist is a floor ensuring baseline coverage, not a ceiling capping what a thoughtful reviewer should consider.
4. Never revisiting or updating a checklist after a review "checklist gap" causes a real incident — a checklist should evolve based on lessons learned, the same way the companion Coding Standards book's Chapter 12 treats a style guide as a living document.

Checklist: using review checklists effectively

1. Maintain both a functional and non-functional checklist, ensuring baseline coverage across correctness AND the broader standards dimensions from Chapter 4.
2. Use the checklist as a memory aid prompting genuine engagement, not a mechanical box-ticking exercise divorced from actually reading the code.
3. Maintain specialized checklist additions for high-risk change categories (migrations, public API changes, security-sensitive code) rather than relying on one generic checklist for everything.
4. Update the checklist when a real incident reveals a gap it didn't previously cover — treat it as a living document, not a fixed artifact.

Discussion / interview questions

1. "Why use a review checklist at all, if experienced reviewers should already know what to check?" Strong answer: Checklists make thoroughness consistent rather than dependent on memory or attention on a given day — even experienced practitioners in high-stakes fields (aviation, medicine) rely on checklists specifically because human memory is unreliable under real-world conditions, not because they don't know the material.

2. "What's the risk of using a review checklist mechanically?" Strong answer: A reviewer can tick every box without genuinely engaging with the specific code, satisfying the checklist's letter while missing its actual purpose — the checklist is meant to prompt real thought about each dimension, not substitute for it (directly related to the rubber-stamping anti-pattern).

3. "Should every PR be reviewed against the same checklist, regardless of what kind of change it is?" Strong answer: No — a generic checklist misses category-specific risks; database migrations, public API changes, and security-sensitive code each have distinct failure modes worth a specialized checklist addition beyond the generic baseline.

4. "When would you consider updating your team's review checklist?" Strong answer: After an incident or missed issue reveals a gap the existing checklist didn't cover — treating the checklist as a living document that evolves from real, observed lessons, similar to how a style guide should evolve (companion Coding Standards book, Chapter 12).

Chapter summary / cheat sheet

• A functional checklist covers correctness, edge cases, error handling, and test coverage; a non-functional checklist covers naming, security, performance, design fit, and documentation — together spanning Chapter 4's six review dimensions.
• A checklist ensures baseline coverage (a floor); it's not a substitute for genuine engagement with the specific code (not a ceiling) — mechanical box-ticking is a real anti-pattern to avoid.
• Maintain specialized checklist additions for high-risk change categories (migrations, public API changes) rather than relying on one generic checklist for everything.
• Treat the checklist as a living document, updated when a real incident reveals a gap it didn't cover.

---