You need a one-line bug fix in a 600-line file full of dead code, misleading names and no tests. How do you honour "leave it cleaner than you found it" without making the change risky to ship or painful to review?
answer
- Fix commit first, minimal, revertable
- Characterization test before restructuring
- Clean only the region you had to read
- Cleanup diff ≤ fix diff, else separate PR
- Ticket the rest; no bare TODOs
basics
~20 sFix the bug first in its own small commit. Then add one or two safe, behaviour-preserving cleanups — delete dead code, rename a local, add a test — as separate commits. Leave the rest; note it, don't do it all.
solid answer
~50 sSplit by intent, not by file. Land the fix as a minimal, self-contained commit so it can be reviewed, cherry-picked and reverted independently — this matters most if it's a production hotfix. Then add cleanup as separate commits, ordered so each is individually verifiable: add a characterization test for the behaviour you're touching, delete provably-dead code, rename locals with an automated refactoring tool, extract a named function from the block you had to read anyway. Bound the effort: roughly, cleanup diff should not dwarf the fix, and should stay inside the region you actually had to understand. Everything else gets recorded — a ticket, a TODO with a ticket id, or a note in the review — not done. Two rules keep this healthy: cleanups must be behaviour-preserving, and style must be owned by an auto-formatter so review time is spent on meaning rather than whitespace.
code
text · 9 linesPR: "Fix off-by-one in dormancy check"
c1 fix: use >= 90 days in isDormant (3 lines) <- revertable alone
c2 test: characterize dormancy boundary cases (40 lines)
c3 refactor: delete unused legacy formatter (-55 lines)
c4 refactor: rename d -> daysSinceLastLogin (tool rename)
# NOT done, ticketed instead:
# PROJ-4412 split 600-line UserBatch into policy + IOgo deeper
Say you'd fix the bug in its own small commit, make one or two obvious safe cleanups (dead code, a rename), run the tests, and leave the rest alone.
Add the mechanics: separate commits, characterization test before touching untested structure, clean only the region you had to read, ticket what you skip, let the formatter own style.
Argue the trade-off explicitly — review capacity, revertability, cherry-picking a hotfix, merge-conflict cost against in-flight branches — and give the heuristic for when cleanup graduates to its own PR merged first.
Talk about removing the judgement call from individuals: shared formatter, new-code-only quality gates with baselines, blame-ignore files for mechanical commits, and review norms that treat a mixed fix+refactor commit as a review defect.
## The tension The Boy Scout Rule ("leave the code cleaner than you found it") collides with two other real constraints: - **Reviewability** — reviewers have a finite attention budget. Empirical review guidance puts effective review at a few hundred changed lines; past that, defect detection drops sharply. A 900-line diff containing one real logic change is *less* safe than a 3-line diff, even though it's "cleaner". - **Revertability** — production incidents are resolved by reverting. A commit that mixes a fix with restructuring cannot be cleanly reverted or cherry-picked into a release branch. So the skill being tested is not "do you tidy" but **"do you know how to bound tidying"**. ## Vocabulary - **Commit** — one atomic recorded change. **PR / merge request** — a set of commits proposed for merging, the unit of human review. - **Cherry-pick** — copying one commit onto another branch (e.g. pulling just a hotfix onto a release branch). - **Blast radius** — how much of the system a change could break. - **Characterization test** (Michael Feathers, *Working Effectively with Legacy Code*) — a test written not to assert *correct* behaviour but to *record current* behaviour, so later refactoring can detect accidental changes. Feathers famously defines legacy code as "code without tests". - **Seam** — a place where you can change behaviour without editing in place (e.g. injecting a dependency), used to get untested code under test. - **Dead code** — code no reachable path executes. "Provably dead" means the compiler, a static analyser, or exhaustive call-graph search says so — not just "I don't think it's used", which is how you delete a reflectively-invoked handler. ## The procedure **Step 0 — decide if now is the time at all.** If this is a production hotfix at 2 a.m., the answer is: fix only, cleanup later, no exceptions. Urgency is a legitimate reason to skip the rule (see failure modes below). **Step 1 — make the fix minimal and first.** One commit, only the lines the bug requires. It should be readable in isolation and revertable in isolation. **Step 2 — build the safety net for anything structural.** In an untested 600-line file, any restructuring is unverified. So the first cleanup commit is usually a **characterization test** covering the path you touched. This is doubly valuable: it's the highest-leverage missing artifact in the file, and it licenses the rest of the cleanup. **Step 3 — apply cleanups in ascending risk order, one concern per commit:** 1. delete provably-dead code and commented-out blocks 2. delete or correct comments that contradict the code 3. rename **locals and parameters** (use the IDE/tooling refactoring, not find-and-replace) 4. replace magic literals with named constants 5. extract and name the block you had to decipher to find the bug 6. flatten nesting with guard clauses / early returns Stop climbing when the next step needs coordination with other people's in-flight work. **Step 4 — bound the scope with explicit heuristics.** Useful, defensible ones: - Clean only what you had to *read* to make the fix — the "understood region", not the whole file. - Keep the cleanup diff comparable to or smaller than the fix diff; if it's growing past that, it deserves its own PR. - Time-box it (say 15–30 minutes). Beyond that it's a planned refactoring, not Boy Scouting. - Don't touch code someone else has a large branch open on — check for in-flight work first. **Step 5 — record what you didn't do.** The mess you *saw* is valuable information. Log a ticket referencing the file and the specific smell, or leave a `TODO(TICKET-123):` marker. An unqualified bare `TODO` with no owner or id is itself a smell — it will be there in five years. ## Same PR or separate PR? | Situation | Recommendation | |---|---| | Cleanup is a handful of lines, in the same region | Same PR, separate commits | | Cleanup is large but mechanical (rename, dead-code removal) | Separate PR, merged **before** the fix, so the fix diff stays clean | | Hotfix / incident | Fix only; cleanup in a follow-up PR | | Cleanup requires a design decision | Not Boy Scouting — ticket + discussion | Merging cleanup *first* is often better than after: the fix then lands against already-tidy code, and any conflict pain is paid on the mechanical change rather than the behavioural one. ## What a reviewer should push back on - Commits that mix a rename with a logic change (the rename hides the logic change in the diff). - Whitespace/format churn that isn't produced by the shared formatter. - Renaming public/exported symbols in a bug-fix PR — cross-module blast radius. - Restructuring untested code with no test added. - "Drive-by" behaviour improvements ("I also made it retry") smuggled in as cleanup. ## Interaction with tooling Most of this becomes cheap with the right tooling and is worth naming: - **Auto-formatter** run in CI or on save: removes the entire class of style arguments. - **"New code only" quality gates** (analysers that fail the build on newly-introduced issues while grandfathering existing ones via a baseline): they encode the Boy Scout Rule mechanically — the codebase can only get better on the lines you touch. - **Automated refactoring tools**: a tool-driven rename is essentially risk-free compared to a textual one. - **Ignore-revs / blame-ignore files**: let bulk mechanical commits be skipped by line-history tools, removing the "it destroys blame" objection. ## How to answer Lead with **separation** (fix first, cleanup separately), then **safety net** (characterization test before restructuring untested code), then an explicit **bound** (understood region, time box, diff size), then **record the rest**. Finish by naming when you'd skip cleanup entirely: hotfixes, code scheduled for deletion, files with heavy in-flight branches, and anything requiring a design decision.
- The cleanup you want to do would conflict with a large refactoring branch a teammate has open. What now?Skip it and coordinate. Note the intent on their branch or ticket. Merge-conflict cost is paid by a human under time pressure, which is riskier than leaving the mess a week longer.
- Your team's reviewers complain that mixed commits make review hard, but your version control workflow squashes everything on merge. How do you keep the benefit?Either split into separate PRs (each squashing to one coherent commit), or turn off squash for PRs where commit separation carries meaning. If everything squashes, the separation must move up a level to the PR.
- How do you stop "cleanup" from silently becoming a behaviour change?Cleanups must be refactorings — verified by tests that pass unchanged before and after. If a test had to change, the commit changed behaviour and must be relabelled and reviewed as such.
A plumber called for a leaking tap wipes the cabinet under the sink and throws out the rusted spare parts — he does not re-pipe the bathroom. And he tells you the pipes need work rather than starting on it uninvited.
saying these in an interview costs you the question
- Shipping a hotfix and a large refactor in one commit, making revert or cherry-pick impossible
- Restructuring untested legacy code without first pinning behaviour with a characterization test
- Deleting code assumed dead without proving it (reflection, dynamic dispatch, config-driven wiring)
- Reformatting the entire file so the actual fix is invisible in the diff
- Leaving bare TODOs with no ticket or owner as a substitute for recording the debt
- Treating "leave it cleaner" as "leave it clean" and blocking the fix until the file is perfect