--- name: safe_refactoring description: "Refactoring code without changing what it does: locking current behavior with characterization tests before touching structure, working in small steps that each end green, naming the smell being removed, and stopping rules for when a refactor has quietly turned into a rewrite. Use when cleaning up, restructuring, de-duplicating, or modernizing existing code, when a function or class has grown hard to change, or before adding a feature to tangled code." --- # Safe Refactoring ## Purpose Change the internal structure of code without changing its observable behavior. That is the whole contract, in Martin Fowler's definition. The value is not cleaner code for its own sake. The value is code that is cheaper to change next time, with proof that today's behavior survived. Use this skill when the code works and the problem is its shape: a long function, copied logic, a class that changes for five reasons. Do not use it to fix bugs or add features in the same pass. Kent Beck's two hats rule, as Fowler teaches it, says you wear one hat at a time: adding function, or refactoring. Switch hats as often as you like, but never wear both at once. This skill ships working tools, not just prose: - `scripts/refactor_gate.py` runs the tests for one step and appends a GREEN or RED line to a log. - `examples/before.py`, `examples/after.py`, and `examples/test_receipt.py` are a complete worked refactor. The same characterization tests pass against both versions, unchanged. - `references/smells-and-moves.md` maps each smell group to the move that removes it. ## Workflow ### 1. State the job in one sentence Write down: the smell, the move, and the behavior that must not change. Good: "Duplicated Code in the receipt builder. Extract the discount tiers into one function. Receipt totals and text stay identical." Bad: "Clean up the billing module." That has no edge, so it has no stopping point. If you cannot name a smell, read `references/smells-and-moves.md` and match the code against the groups: Bloaters, Object-Orientation Abusers, Change Preventers, Dispensables, Couplers. If nothing matches, the code may not need refactoring. Say so and stop. ### 2. Lock current behavior before touching structure Run the existing test suite first and record the result. If it is already red, report that. A refactor that starts red can never prove it broke nothing. Where coverage is thin or missing, write characterization tests. Michael Feathers coined the term in Working Effectively with Legacy Code. A characterization test describes what the code actually does now, not what it should do: - Run the code with real inputs. Record the actual outputs, including the odd ones. - Pin those outputs in a test. If an output looks like a bug, pin it anyway and add a comment marking it as suspected. Fixing it is a separate job, under the other hat. - Cover the paths you are about to move, plus at least one boundary on each side of every branch you will touch: empty input, the value just below a threshold, the value at it, the value above it. - For whole-output code such as reports or receipts, save one full output as an exact-match test. This is the golden master form of the same idea. It catches drift that field-by-field checks miss. Worked example: `examples/test_receipt.py` was written against `before.py` this way. One expectation was first written from reading the code, tax of $9.60 on a $120 order. Running the code showed the real tax is $8.64, charged on the discounted amount. The test was corrected to the actual behavior before any refactoring started. That correction is the method working, not a detour. Run the gate on the untouched code: ``` python3 scripts/refactor_gate.py --label "baseline: characterization tests on original code" -- python3 -m unittest test_receipt.BeforeReceipt -v ``` Verified output for the example: 5 tests ran, OK, gate line logged GREEN. Do not proceed on a RED baseline. ### 3. Work in small steps that each end green One move per step. One step per commit where commits are available. - Apply a single named move: Extract Function, Rename Variable, Move Function, Inline Variable, Replace Temp with Query, or another catalog move from the reference file. - Run the tests through the gate, with a label that names the smell and the move. - GREEN: commit or record the step, then take the next step. - RED: revert that step. Do not debug forward through a broken refactor unless the failure is a typo you can point at. A revert costs minutes. Debugging a half-moved structure costs hours and muddies the proof. - Keep steps small enough that a revert loses little. If a step needs more than about an hour of edits before the next test run, split it. Rename steps travel alone. A rename mixed into a structural change makes review and revert painful, so do renames as their own gated steps. ### 4. Use the right pattern when one move is not enough - **Preparatory refactoring:** when a planned feature is hard to add, refactor first until the feature looks like a small addition, then switch hats and add it. Two passes, two proofs. - **Parallel change, also called expand and contract:** when a shared shape must change, such as an API field, a stored format, or a function signature with many callers. Expand by supporting old and new shapes at once. Migrate each caller. Contract by removing the old shape only after a search shows no user left. - **Branch by abstraction:** introduce a call through a new seam, build the new implementation behind it, switch the seam over, then remove the old code. - **Strangler fig:** for replacing a whole component or service. Fowler coined the term in 2004 for gradual replacement around a legacy system: put a facade or routing seam in front, move one slice of behavior at a time to the new side, verify each slice, and retire the old path only when its traffic is gone. Transform, coexist, eliminate. ### 5. Run the stopping rules Stop refactoring and reassess when any of these trip: - **Behavior changed on purpose.** The moment a step is meant to alter output, it is no longer a refactor. Stop, switch hats, and handle it as a feature or bug fix with its own tests. - **The tests cannot be made green on the original code.** You have no net. Either invest in characterization tests first, or admit the work is a rewrite and plan it as one, with a parallel run or a rollout plan. - **Two RED steps in a row on the same move.** The move does not fit the code as it stands. Back out and pick a smaller preparatory move. - **The diff touches files outside the stated job.** Scope crept. Split the extra work into its own job instead of letting it ride along. - **Callers keep appearing.** If each extraction reveals another layer of users you did not know about, the module boundary is wrong. Stop and map the callers before moving more code. - **The estimate passed a full rewrite of the unit.** A rewrite is a legitimate choice, but it is a different job with different risk: no behavior proof by construction, so it needs its own plan, review, and acceptance tests. Name it honestly and get it approved as a rewrite. - **The smell is gone.** The job was the smell, not perfection. Stop when the named smell is removed and the code reads plainly. Polishing past that point adds risk with no named payoff. ### 6. Close out - Run the full suite, not just the touched tests, through the gate one last time. - Diff the public surface before and after: function signatures, output formats, file formats, error messages. Anything user-visible that changed needs a deliberate explanation or a revert. - Delete characterization tests that only duplicated good permanent tests. Keep the ones that pin behavior nothing else covers, and rename them so they read as behavior tests, not scaffolding. - Attach the gate log to the change. It is the evidence that each step ended green. ## Output Contract Report the refactor in this shape: ``` Refactor: Baseline: Steps: - | GREEN | - | GREEN | Final: Public surface diff: Stopped because: Not done: ``` For the shipped example, filled in with real runs: ``` Refactor: Duplicated Code and Long Function in receipt builder. Extract discount, shipping, and tax into named functions. Receipt text and totals stay identical. Baseline: python3 -m unittest test_receipt.BeforeReceipt -v | 5 tests | GREEN Steps: - Duplicated Code + Long Function: extract discount_for, shipping_for, tax_for, line_total | GREEN | 10 tests across before and after Final: python3 -m unittest test_receipt -v | 10 tests | OK Public surface diff: none. build_receipt signature and output are byte-identical. Stopped because: named smells removed, output pinned by exact-match test. ``` ## Operating Rules - Never refactor on a red baseline and claim safety. Red first means the proof chain is broken from the start. - Never mix a behavior change into a refactoring step, even a small fix spotted on the way. Write it down, finish the step, then switch hats. - Never skip the test run because a step looks trivial. Renames break callers. Extractions miss a variable. The gate run is cheap, the surprise is not. - Prefer reverting a RED step over fixing forward. The previous green state is the most valuable thing in the process. - Treat unverified behavior as unknown behavior. If no test and no run can show what the code does today, say that plainly in the report instead of guessing. - Delete dead code only after a search covers dynamic calls, reflection, configuration references, and external callers where they apply. When in doubt, deprecate in one pass and delete in a later one. - Keep the gate log. A refactor without a step log asks the reviewer to trust you. A refactor with one shows its work.