Chapter 13: Worked Example: Reviewing a Real, Flawed Pull Request

Why this matters

Every principle in this book has been demonstrated in isolated pieces. This chapter reviews one realistic, moderately-sized PR end to end — applying Chapter 4's six dimensions, Chapter 5's checklist discipline, and Chapter 6-7's tone and phrasing guidance together, exactly the way a real review (or a code-review-focused interview exercise) requires.

The PR under review

PR title: "Add discount code redemption to checkout"

def apply_discount_code(order, code):
    discount = db.query(f"SELECT * FROM discount_codes WHERE code = '{code}'")
    if discount:
        if discount.uses_remaining > 0:
            order.total = order.total - discount.amount
            db.execute(f"UPDATE discount_codes SET uses_remaining = uses_remaining - 1 WHERE code = '{code}'")
        return order
    else:
        return order

def checkout(order, discount_code=None):
    if discount_code:
        order = apply_discount_code(order, discount_code)
    charge_payment(order)
    return order

PR description: "Adds discount codes to checkout."

Applying the review, dimension by dimension

PR description (Chapter 3): "The description is quite thin — could you expand it to cover how discount codes are validated (e.g., is there a minimum order amount, can codes be combined, what happens with an invalid code) and how this was tested? This'll help reviewers and future readers understand the intended behavior, not just the mechanical change."

Correctness: "BLOCKING: order.total = order.total - discount.amount doesn't check whether this could make total negative — if a discount amount exceeds the order total, this would produce a negative charge. Should there be a floor at zero, or should a discount larger than the order be rejected entirely?"

Concurrency/correctness — a check-then-act race (companion Concurrency book, Chapter 3): "BLOCKING: This checks uses_remaining > 0 and then separately decrements it — two concurrent requests using the same discount code could both pass the check before either decrements, allowing the code to be used more times than uses_remaining should permit. This needs a transaction with row locking (SELECT ... FOR UPDATE), similar to the pattern in OrderService.reserveStock()."

Security (companion Coding Standards book, Chapter 10): "BLOCKING: Both the SELECT and UPDATE queries build SQL via f-string interpolation of code, which is user-supplied input from the checkout flow — this is a SQL injection vulnerability. Needs parameterized queries: db.query(\"SELECT * FROM discount_codes WHERE code = %s\", (code,))."

Error handling (companion Coding Standards book, Chapter 5): "nit: apply_discount_code silently returns the unmodified order for both 'no such code' and 'code exists but the DB query threw an exception' — these are different situations from the caller's perspective. Consider distinguishing an invalid code (expected, should probably surface a user-facing message) from an actual system error (should probably be logged and possibly fail the checkout, depending on our fail-fast/fail-gracefully convention for this flow)."

Tests: "I don't see any tests included — given this touches payment/checkout logic, I'd want to see tests covering: a valid code, an invalid code, a code with zero uses remaining, and (once the concurrency issue above is fixed) a concurrent-redemption test. Happy to pair on the concurrency test specifically if useful, since it's a bit more involved to set up."

Design fit: "nit: Other checkout-related database access in this codebase goes through OrderRepository rather than calling db.query/db.execute directly (see OrderService) — might be worth following that pattern here for consistency, and it would likely make the parameterized-query and transaction fixes above more natural to implement within the existing repository structure."

Why this review works as a model

[SENIOR INSIGHT] Notice what this review demonstrates simultaneously: every blocking comment states specifically what goes wrong (Chapter 11's gatekeeping countermeasure — "what specifically fails if this merges as-is?" is answerable for every one of them), nits are explicitly labeled and clearly optional (Chapter 6), tone is specific and non-accusatory throughout (naming the code pattern, not the author's competence), and the review covers all six dimensions from Chapter 4 rather than stopping once the security issue was found (a very human, but incomplete, stopping point — finding one serious issue doesn't mean the rest of the review can be skipped).

Common mistakes

1. Stopping the review once the first serious issue (like the SQL injection) is found, missing the concurrency bug, the missing negative-total check, and the testing gap that a complete, six-dimension review would have caught.
2. Bundling all the feedback into one long, undifferentiated comment rather than clearly separating blocking issues from nits, making it hard for the author to prioritize what must change before merge versus what's optional.
3. Phrasing the security and concurrency findings as personal criticism ("you didn't think about concurrency here") rather than the specific, depersonalized, actionable phrasing shown above.
4. Not offering to help with the harder fixes (like the concurrency test) — a review that only identifies problems without acknowledging their relative difficulty or offering support can read as unnecessarily harsh, especially for a genuinely tricky fix like a concurrency-safe redemption flow.

Discussion / interview questions

1. "Review this discount code function and identify the issues you'd raise." Strong answer: Identifies the SQL injection, the check-then-act concurrency race, the missing negative-total guard, the thin PR description, and the missing tests — covering multiple dimensions rather than stopping after the first (often most visually obvious) issue found.

2. "How would you prioritize and label the issues you found in this review?" Strong answer: Labels the SQL injection, concurrency race, and negative-total bug as BLOCKING (each has a concrete, statable failure mode), and the error-handling distinction and repository-pattern consistency as nits — explicitly distinguishing what must be fixed before merge from what's a matter of preference.

3. "How would you phrase the concurrency issue you found, applying the tone principles from earlier in this book?" Strong answer: Names the specific mechanism (check-then-act on uses_remaining), states the concrete failure scenario (two concurrent redemptions both passing the check), and suggests a specific fix referencing an existing pattern in the codebase — specific, non-accusatory, and actionable, rather than a vague "this isn't thread-safe."

4. "This PR has several real problems. How would you balance being thorough with not overwhelming the author?" Strong answer: Clearly separates blocking issues (which must be addressed) from nits (which are optional), offers to pair on the more difficult fixes, and frames feedback around the code's behavior rather than the author's competence — thoroughness and a supportive, collaborative tone aren't in tension when the feedback is structured and phrased well.

Chapter summary / cheat sheet

• A complete review applies all six dimensions from Chapter 4 even after finding one serious issue — don't stop at the first (often most visible) problem.
• Every blocking comment should answer "what specifically goes wrong if this merges as-is?" — this is both a quality bar for the comment and a gatekeeping countermeasure (Chapter 11).
• Explicitly separate blocking issues from nits, and phrase all feedback specifically, non-accusatorially, and actionably (Chapters 6-7).
• Offering to help with harder fixes (pairing, specific references to existing patterns) turns a thorough review from merely "correct" into genuinely collaborative — serving Chapter 1's mentorship and shared-ownership purposes at the same time as its quality purpose.

---