Chapter 11 · Reviewer

Reading a diff, finding the bug

diff review — spot the off-by-one / wrong operatorREV-10174 of 84

A diff is a view of what changed: removed lines marked -, added lines marked +, with a few unchanged lines around them for context. Reviewing one means reading the *new* code as if you have to own it, not skimming for typos.

The most common real bugs in a diff aren't crashes — the code runs fine and returns the *wrong answer*. Watch for three classic families:

Off-by-one — < where you meant <= (or range(n) vs range(n+1)). The loop or check is one step short or one step long. Boundaries are where these hide.
Wrong operator — and where it should be or, + where it should be -, > flipped to <. The shape looks right; the logic is inverted.
Inverted condition — if not ready instead of if ready, or a branch that returns the wrong one of two values.

The technique that catches all of them: trace it by hand on concrete values. Pick an input below the boundary, one on it, one above — and step through what the code actually returns versus what it *should*. Don't trust that it looks right; run the numbers in your head.

Syntax

# Reviewer's diff checklist — trace, don't skim:
#  1. What is this function SUPPOSED to return? (read the name/docstring)
#  2. Pick 3 inputs: below the boundary, ON it, above it.
#  3. Step through each branch by hand — what comes out?
#  4. Suspect operators: <  vs <=   >  vs >=   and vs or   + vs -
#  5. Suspect each `return`: is it returning the RIGHT variable?

Worked examples

Off-by-one: a capacity check that's one short

# BUGGY — rejects the last legal entrant:
def has_capacity(count, limit):
    return count < limit       # at count == limit it says False

print(has_capacity(10, 10))    # False  <-- wrong: 10 of 10 IS allowed

# FIXED — the limit is inclusive:
def has_capacity(count, limit):
    return count <= limit

print(has_capacity(10, 10))    # True

Wrong return in a branch (the REV-101 clamp bug)

# BUGGY — a value below the floor is pushed to the CEILING:
def clamp(value, low, high):
    if value < low:
        return high            # <-- wrong variable
    if value > high:
        return high
    return value

print(clamp(-5, 0, 10))        # 10  <-- should be 0

# FIXED — below the floor clamps UP to `low`:
def clamp(value, low, high):
    if value < low:
        return low
    if value > high:
        return high
    return value

print(clamp(-5, 0, 10))        # 0

Wrong operator: and where the rule needs or

# BUGGY — only lets staff in, never the account owner:
def can_edit(is_staff, is_owner):
    return is_staff and is_owner   # demands BOTH

print(can_edit(False, True))       # False  <-- owner locked out

# FIXED — either role grants edit:
def can_edit(is_staff, is_owner):
    return is_staff or is_owner

print(can_edit(False, True))       # True

Common mistakes & gotchas

Skimming for style, missing the logic

It's easy to comment on spacing and naming and feel like you reviewed it — while a < that should be <= sails straight through. Trace the actual behaviour on boundary values *first*; cosmetics come last.

Trusting the function name over the code

The function is *called* clamp, so your brain assumes it clamps. Reviewers read what the code DOES, not what it's named. The bug is precisely the gap between the two.

Only checking the happy middle

Testing clamp(5, 0, 10) passes on the buggy version too — the bug only shows below low. A value in the comfortable middle proves nothing. Always probe the edges.

Why it matters

Most production bugs are not crashes — they're code that runs cleanly and quietly returns the wrong number. A reviewer who traces diffs on boundary values catches these before a customer does, which is the entire point of review. It's the cheapest place in the whole pipeline to find a bug.