Chapter 4: Reviewer-Side Practices: What to Actually Check

Why this matters

"Review this PR" is an underspecified instruction — a thorough review touches several genuinely distinct dimensions (correctness, tests, readability, security, performance, design fit), and a reviewer who only checks one or two of these (correctness is the most common default) leaves the others unguarded, believing the review was thorough when it covered only a fraction of what review can catch.

The six dimensions of a thorough review

[BEGINNER]

1. Correctness — does the code actually do what it's supposed to do, including edge cases the happy-path logic might miss?
2. Tests — are the changes adequately tested, and do the tests actually verify meaningful behavior (not just exercise the code without real assertions)?
3. Readability — could another engineer (or future-you) understand this code without extensive explanation, per the companion Coding Standards book's Chapters 2 and 4?
4. Security — does this change introduce or fail to guard against a vulnerability class (companion Coding Standards book, Chapter 10 — injection, unvalidated input, exposed secrets)?
5. Performance — does this change introduce an obvious performance problem (an N+1 query, an unbounded loop over user-controlled input, an unnecessary synchronous blocking call) that wouldn't show up in a quick functional test but would matter at real scale?
6. Design fit — does this change fit coherently with the codebase's existing architecture and patterns, or does it introduce an inconsistent, one-off approach to a problem the codebase already has an established solution for?

[SENIOR INSIGHT] Not every PR needs equal depth across all six dimensions — a one-line copy change needs essentially none of them beyond a glance; a new payment-processing code path needs deep attention to correctness, security, and design fit specifically. Recognizing which dimensions matter most for a specific PR, rather than mechanically applying uniform depth everywhere, is itself the actual reviewing skill — spending equal, moderate effort on all six for every PR regardless of context wastes effort on trivial changes and under-invests in high-stakes ones.

A worked example: reviewing the same PR across each dimension

def get_user_orders(user_id, page_size=1000):
    orders = db.query(f"SELECT * FROM orders WHERE user_id = {user_id}")
    return orders[:page_size]

Correctness: "This truncates to page_size but doesn't actually paginate — calling this repeatedly always returns the same first N orders. Is pagination actually needed here, or is page_size misleadingly named for what's really just a result cap?"

Tests: "I don't see a test for the case where a user has more orders than page_size — worth adding, since that's exactly the case most likely to reveal the pagination issue above."

Readability: "Consider renaming page_size to max_results if pagination isn't actually implemented — the current name implies a capability the function doesn't have."

Security: "This builds a SQL query via f-string interpolation with user_id — even if user_id currently only comes from an authenticated session, this is a SQL injection risk pattern (see our Coding Standards guide, Chapter 10) and should use a parameterized query regardless of the current caller."

Performance: "This fetches all matching rows before truncating in Python ([:page_size]) — for a user with a very large order history, this pulls far more data from the database than needed. Consider a LIMIT clause in the SQL itself instead."

Design fit: "Other repository functions in this codebase use the QueryBuilder utility rather than raw SQL strings (see OrderRepository for an example) — worth using that here for consistency, and it would also resolve the security issue above as a side effect."

This one small function generated six genuinely distinct, valuable comments — a reviewer checking only correctness would have caught the pagination logic issue but missed a real security vulnerability, a performance problem, and a design-consistency issue entirely.

Reading the diff in context, not in isolation

[IN PRACTICE] A common mistake even among reviewers who know to check all six dimensions: evaluating a diff purely on its own, without pulling up the surrounding, unchanged code it interacts with. A new function might look correct in isolation but subtly break an assumption an existing caller relies on — catching this requires actually looking at how the changed code is used elsewhere, not just reading the highlighted diff lines in the review tool.

Common mistakes

1. Reviewing only for correctness, the most intuitive and commonly-defaulted-to dimension, while missing readability, security, performance, and design-fit issues entirely.
2. Applying uniform review depth regardless of the PR's actual stakes — spending significant time on a trivial change while under-investing in a high-risk one (a new payment path, an auth change) is a real, common misallocation of reviewer effort.
3. Reviewing the diff in isolation without checking how the changed code interacts with its surrounding, unchanged context — missing subtle breakages that only become visible by looking at actual callers/usages.
4. Treating test presence alone as sufficient, without evaluating whether the tests actually verify meaningful behavior — a test that runs code without asserting anything meaningful provides false confidence.

Checklist: reviewing thoroughly across all dimensions

1. Explicitly check correctness, tests, readability, security, performance, and design fit for any non-trivial PR — don't let correctness alone stand in for a complete review.
2. Calibrate depth to the PR's actual stakes — spend real time on high-risk changes (auth, payments, data integrity); a lighter pass is fine for low-stakes changes.
3. Look at how changed code is actually used elsewhere in the codebase, not just the highlighted diff in isolation, to catch subtle breakages to existing assumptions.
4. Evaluate whether tests actually verify meaningful behavior, not just whether tests exist at all.

Discussion / interview questions

1. "What are the different dimensions you'd check when reviewing a pull request, beyond 'does this work'?" Strong answer: Names correctness, tests, readability, security, performance, and design fit as genuinely distinct dimensions, and explains that checking only correctness (the most common default) leaves the other five entirely unguarded.

2. "How would you decide how much time/depth to invest in reviewing a specific PR?" Strong answer: Calibrates based on the PR's actual stakes — a trivial change needs minimal review time, while a change touching authentication, payments, or core data integrity warrants deep attention across all six dimensions; applying uniform depth everywhere misallocates reviewer effort.

3. "Why is it important to look at how changed code is used elsewhere in the codebase, not just the diff itself?" Strong answer: A change can look correct in isolation but subtly violate an assumption an existing caller relies on — this kind of breakage is only visible by examining actual usages, not by reading the highlighted diff lines alone.

4. "Is the presence of tests in a PR sufficient evidence that the change is well-tested?" Strong answer: No — a test can execute code without asserting anything meaningful, providing false confidence; a thorough reviewer evaluates whether the tests actually verify the behavior that matters, not just whether test files were included.

Chapter summary / cheat sheet

• A thorough review checks six genuinely distinct dimensions: correctness, tests, readability, security, performance, and design fit — correctness alone (the most common default) leaves the other five unguarded.
• Calibrate review depth to a PR's actual stakes, not uniformly — high-risk changes (auth, payments, data integrity) deserve deep attention across all dimensions; trivial changes don't.
• Review changed code in the context of its actual usages elsewhere in the codebase, not just the isolated diff.
• Evaluate whether tests verify meaningful behavior, not just whether they exist.

---