Chapter 11 · Reviewer

Judging naming and structure

behaviour-preserving refactor — extract a named helperREV-10376 of 84

Not every review finds a bug. Often the code is *correct* but hostile to read — single-letter names, a tricky rule crammed into one line, the same condition copy-pasted in three places. The reviewer's ask here is a behaviour-preserving refactor: change the shape, not the output.

The most valuable move is extracting a named helper. When a condition or calculation has a *meaning* ("is this order worth chasing?"), pull it into a function whose name states that meaning. The call site then reads like a sentence: if is_high_value(order). The logic stops being a puzzle.

What makes a good name and a good shape:

A name says what, not how — is_high_value, not check_status_and_total.
Single responsibility — one function, one job. A function that filters *and* formats *and* saves is three functions wearing a trench coat.
No magic repeated inline — if the same x["status"] == "open" and x["total"] > 100 appears twice, name it once.

The iron rule: a refactor is only safe when behaviour is *identical*. Same inputs, same outputs, every time. If you find yourself "fixing" a result while you tidy, stop — that's a behaviour change, and it needs its own review, its own test, and its own discussion. Refactor or fix, never both silently in one move.

Syntax

# Extract-a-helper refactor — same behaviour, clearer intent:

# BEFORE: meaning buried in a one-liner
def f(d):
    return [x for x in d if x["status"] == "open" and x["total"] > 100]

# AFTER: the rule has a name; the filter reads like English
def is_high_value(order):
    return order["status"] == "open" and order["total"] > 100

def high_value_orders(orders):
    return [o for o in orders if is_high_value(o)]

Worked examples

Cryptic but correct → named and clear (REV-103)

# BEFORE — works, but `f` and `d` and `x` tell you nothing:
def f(d):
    return [x for x in d if x["status"] == "open" and x["total"] > 100]

# AFTER — extracted helper, meaningful names, identical output:
def is_high_value(order):
    return order["status"] == "open" and order["total"] > 100

def high_value_orders(orders):
    return [o for o in orders if is_high_value(o)]

orders = [
    {"status": "open",   "total": 150},
    {"status": "closed", "total": 500},
    {"status": "open",   "total": 50},
    {"status": "open",   "total": 101},
]
print(high_value_orders(orders))
# [{'status': 'open', 'total': 150}, {'status': 'open', 'total': 101}]
# (closed is excluded; the 50 and the boundary are excluded — > 100 is strict)

Proving behaviour is preserved

# A reviewer confirms a refactor by checking old == new on real data:
def f(d):
    return [x for x in d if x["status"] == "open" and x["total"] > 100]

def is_high_value(order):
    return order["status"] == "open" and order["total"] > 100

def high_value_orders(orders):
    return [o for o in orders if is_high_value(o)]

sample = [{"status": "open", "total": 150}, {"status": "open", "total": 100}]
print(f(sample) == high_value_orders(sample))   # True  — same behaviour
print(is_high_value({"status": "open", "total": 100}))  # False — 100 is NOT > 100

Single responsibility: split the function that does too much

# BEFORE — one function filters AND formats (two jobs):
def report(orders):
    big = [o for o in orders if o["total"] > 100]
    return "\n".join(f"#{o['id']}: {o['total']}" for o in big)

# AFTER — each function does one thing, and both are reusable:
def big_orders(orders):
    return [o for o in orders if o["total"] > 100]

def format_orders(orders):
    return "\n".join(f"#{o['id']}: {o['total']}" for o in orders)

data = [{"id": 1, "total": 150}, {"id": 2, "total": 40}]
print(format_orders(big_orders(data)))   # #1: 150

Common mistakes & gotchas

'Fixing' behaviour while you refactor

You spot a bug mid-tidy and quietly correct it in the same change. Now the diff mixes a refactor with a fix, and a reviewer can't tell which line changed the output. Keep them separate commits — refactor (behaviour identical) OR fix (behaviour changes on purpose), never both at once.

Renaming for the sake of it (bikeshedding)

Not every name needs your preferred word. If orders is clear, leave it. Refactors have a cost (review time, merge conflicts, risk); spend it where readability genuinely improves, not on cosmetic preference.

Over-extracting into a maze of tiny functions

Pulling a one-line condition that's used once into its own function can make code HARDER to follow — now you jump around to read it. Extract when something is repeated, complex, or carries real meaning; inline when it's trivial and local.

Why it matters

Code is read far more often than it's written — every future fix, feature, and bug-hunt starts by re-reading what's there. A well-named helper is a comment that can't go stale. Reviewing for structure isn't fussiness; it's protecting every developer (including the author in six months) who has to understand this code next.