Do not extract a helper from a messy record loop. Return values are the wrong freeze point for callers. Pin dict mutation, sort keys, and skip counts first.
Green tests still hide later caller breakage. Nested objects change identity under the loop. A later extract then ships silent behavior drift.
The messy loop you actually inherit
Many repos grow one process_records function over years. It filters, sorts, writes, and mutates shared rows. Nobody owns the implicit contract anymore.
The function returns a list of dicts. Callers also keep the original list around. Both shapes matter after the next refactor.
This example is labeled as unexecuted teaching code. Treat it as a characterization target only. Do not copy it into production as-is.
# example_messy.py — teaching fixture, not production code
DEFAULTS = {"status": "new", "retries": 0}
def process_records(rows, min_score=10, reverse=False):
kept = []
skipped = 0
for row in rows:
if not row.get("id"):
skipped += 1
continue
row.setdefault("status", DEFAULTS["status"])
row["retries"] = row.get("retries", DEFAULTS["retries"]) + 1
score = row.get("score", 0)
if score < min_score:
skipped += 1
continue
kept.append(row)
kept.sort(key=lambda r: (r.get("score", 0), r.get("id")), reverse=reverse)
return kept
The loop mutates rows in place on purpose. It increments retries even for skipped scores. Sort uses a two-field tuple key.
A naive extract often rebuilds those dicts. Identity, skip timing, and sort stability then drift. Return-only tests stay green through that drift.
Freeze three observables, not the return list
Return lists hide aliasing bugs in callers. Characterization tests must pin side effects directly. Use object identity, not deep equality alone.
Pin these three facts before any cut:
- Input dict identity after the call.
- Sort key order on equal scores.
- Skip paths and which rows still mutate.
Write tests against the inherited module first. Do not improve behavior while pinning facts. Characterization means freeze, then change once.
# test_characterize_process_records.py — proposal tests
from example_messy import process_records
def test_mutates_original_row_identity():
row = {"id": "a", "score": 20}
out = process_records([row])
assert out[0] is row
assert row["retries"] == 1
assert row["status"] == "new"
def test_increments_retries_before_score_skip():
row = {"id": "b", "score": 2, "retries": 4}
out = process_records([row], min_score=10)
assert out == []
assert row["retries"] == 5
def test_missing_id_does_not_mutate_the_row():
row = {"score": 99}
original = dict(row)
out = process_records([row])
assert out == []
assert row == original
def test_sort_is_score_then_id_on_ties():
rows = [
{"id": "c", "score": 10},
{"id": "a", "score": 10},
{"id": "b", "score": 30},
]
out = process_records(rows)
assert [r["id"] for r in out] == ["a", "c", "b"]
def test_reverse_flag_flips_the_tuple_order():
rows = [
{"id": "c", "score": 10},
{"id": "a", "score": 10},
{"id": "b", "score": 30},
]
out = process_records(rows, reverse=True)
assert [r["id"] for r in out] == ["b", "c", "a"]
Run the suite once on the inherited file. Record pass or fail as the baseline. A fail means your mental model is already wrong.
python -m pytest test_characterize_process_records.py -q
Do not patch tests to match a cleaner design. Patch your understanding of the loop instead. The current code is the spec today.
Decision table for the smallest safe change
Use this table before touching production code. Each row is one allowed or blocked move.
| Proposed change | Mutation identity | Sort tuple | Skip timing | Allowed now? |
|---|---|---|---|---|
Rename local kept
|
unchanged | unchanged | unchanged | yes |
Extract record_sort_key(row)
|
unchanged | same tuple | unchanged | yes |
| Build new dicts in a helper | broken | maybe | maybe | no |
| Skip low scores before retries | unchanged | unchanged | broken | no |
Drop missing-id rows via filter
|
maybe | unchanged | broken | no |
Replace list.sort with new sorted
|
broken aliases | check ties | unchanged | not yet |
The smallest safe change is a rename. A pure key function is also allowed. Anything that allocates new dicts is a second change.
Do not combine those changes in one commit. Reviewers cannot see which observable moved. Split until the diff is boring.
Numbered workflow
Follow this order on a messy module. Skip a step and the extract lies.
- Inventory callers of the god function.
- Note who reuses input row objects.
- Add identity assertions for those rows.
- Add sort-order assertions on score ties.
- Add skip-path mutation assertions next.
- Run the suite and freeze the baseline.
- Extract one pure helper only.
- Re-run the same assertions immediately.
- Stop if any identity test moves.
Inventory is not an optional warmup step. Some callers log the input list after processing. They depend on incremented retries remaining visible.
Search before you guess at those callers. A single extract can miss a script under tools/. Frozen tests cannot cover an unseen alias.
rg -n "process_records\(" -g "*.py"
rg -n "retries" -g "*.py"
Record each hit as mutates, reads-after, or return-only. Identity tests are mandatory for the first two. Return-only hits still need sort pins.
Why return-only tests miss this
Deep equality treats two similar dicts as fine. Production code may hold the old object. Logging, caches, and weak keys then disagree.
Sort tests that check a set of ids also miss. Tie order is part of the live contract. JSON dumps and pagination depend on that order.
Skip tests that only check output length miss mutation. Low-score rows still gain extra retries. Downstream retry budgets then shift without a failing test.
An illustrative assertion failure looks like this. The objects match by value and still fail is. That is the signal you want.
E assert {'id': 'a', 'score': 20, 'retries': 1} is {'id': 'a', 'score': 20, 'retries': 1}
E + where {'id': 'a', 'score': 20, 'retries': 1} = out[0]
If your suite never uses is, this class of extract stays invisible. Add identity pins before the helper lands.
After the freeze: one extract
Only now extract a helper from the loop. Keep that helper pure if possible. Do not copy mutation into a new object.
# smallest safe extract — still teaching code
def record_sort_key(row):
return (row.get("score", 0), row.get("id"))
def process_records(rows, min_score=10, reverse=False):
kept = []
skipped = 0
for row in rows:
if not row.get("id"):
skipped += 1
continue
row.setdefault("status", DEFAULTS["status"])
row["retries"] = row.get("retries", DEFAULTS["retries"]) + 1
score = row.get("score", 0)
if score < min_score:
skipped += 1
continue
kept.append(row)
kept.sort(key=record_sort_key, reverse=reverse)
return kept
That extract changes one function name only. Pinned behavior stays under the same tests. A later model-assisted rewrite now has a tripwire.
Free models after pins, not before
A coding model will often clean mutation by default. It returns brand new dicts for cleanliness. That fails callers who alias the old rows.
Disclosure: This article was prepared as part of MonkeyCode's product outreach.
MonkeyCode offers free model access and a free server option. Use both only after the characterization suite exists. Prompt the model to extract record_sort_key and nothing else.
Do not ask the model to simplify process_records. That prompt invites new dict allocations. Paste the failing identity test if output drifts.
Keep the pytest command identical locally and remotely. Same working directory and same file bytes matter. Hash both files if the remote run disagrees.
This article stays useful without that product. The tests are the actual method. The model is optional labor on a frozen spec.
Commands that keep the freeze honest
Hash the module before and after the extract. A helper extract should stay a tiny diff.
sha256sum example_messy.py
python -m pytest test_characterize_process_records.py -q
# perform the one-helper extract, then:
python -m pytest test_characterize_process_records.py -q
git diff --stat
Expect a small git diff --stat result. Large diffs mean you combined unrelated changes. Split the commit until the stat is tiny.
Print object ids in a failing run. Identity bugs show as different id() values.
# proposal debug helper — delete after the freeze
def dump_ids(rows, label):
print(label, [id(r) for r in rows])
Remove that debug helper after the freeze. Do not leave prints inside the extract. Noise hides the next characterization gap.
Limitations
These tests do not pin JSON key order. They also do not pin logging side effects. They do not pin raised exception types.
The skipped counter is currently discarded. Callers may start using it later. Do not expose it during this extract.
DEFAULTS is a shared mutable dict. This suite does not freeze that alias. Pin it in a later change set.
String scores would change sort order silently. Add a type pin if callers send strings. Do not assume int scores forever.
The suite uses one process and one thread. Concurrent mutation needs different probes. Do not claim thread safety from these tests.
Who should not use this approach
Do not use this if the module has no callers. Delete dead code instead of pinning it. Characterization is for living contracts only.
Do not use this for cryptographic code paths. Pinning accidental mutation is the wrong goal there. Design explicit copies and constant-time compares.
Do not use this to justify a large rewrite. The method forbids combined behavior changes. Stop after one helper extract.
Teams without pytest can still freeze observables. The test runner is interchangeable here. The identity assertions are not interchangeable.
What this does not prove
A green suite does not prove good design. It proves the extract did not move pinned facts. Ugly mutation can and should remain for now.
That is the point of a messy-repo cut. Make the next change small and observable. Leave taste for a later commit.
If product needs immutable rows, add a new function. Keep process_records frozen under these tests. Migrate callers one named path at a time.
Top comments (0)