Refactoring Safely

Refactoring safely — changing shape without changing behaviour

Refactoring is a behaviour-preserving transformation. If behaviour changed, that was not a refactor, it was a rewrite with optimistic branding — and the reason large restructures fail is almost always that the two were done in one step.

The rule that makes the rest work

Never restructure and change behaviour in the same commit. Alternate deliberately: refactor (green → green, no behaviour change), then change behaviour (with a new test). When both happen at once, a failing test cannot tell you which half broke it.

Before touching anything: the net

Legacy code is code without tests, regardless of age. So:

1. Characterization tests first. Not tests of what it should do — tests of what it does, including behaviour you consider wrong. Their job is to detect change, not to judge it. Wrong-but-tested behaviour gets fixed later, on purpose, in its own commit.
2. Cover the seams you are about to cut, not the whole file. Coverage of the region under the knife is what matters.
3. Confirm they fail when you break something. A characterization test that passes against deliberately broken code is worse than none — run that check once.
4. Know how to revert. Small commits, one transformation each.

Smells, and what each one actually indicates

Duplicated text expressing unrelated decisions is not a smell. Merging it couples things that had no reason to change together.

The sequence for a large module

Order matters more than technique, and this order is what keeps each step small:

1. Constants and pure helpers out first. They have no state and no dependents to break, and they are what every future slice would otherwise import from the old file.
2. Then the state nobody else touches. Background machinery, caches and locks used by one area move with that area, taking their invariants along.
3. Then whole vertical slices — one feature's handlers plus the state it owns. A slice that takes its own state with it is a clean cut; a slice that leaves state behind produces "modules" that still need the old file.
4. The long tail last. Single functions with no clear home go where they are used, or into one honestly-named remainder. Splitting them first is motion without benefit.
5. Delete an old path only after every identified consumer has migrated and proportionate consumer/integration proof shows the replacement carries the required behavior. For a live externally served boundary, that proof may include traffic/canary evidence; an offline installer, private module, or staged component instead uses its applicable focused contract/integration receipt. Keep the old path until deletion is authorized, needed consumers are proven migrated, and no speculative early removal can erase a rollback or compatibility boundary.

At each step: green before, green after, one commit.

Concurrency: the part that bites

Hand-rolled locks and module-level mutable state do not survive naive moves. Before moving anything guarded:

• Write down the invariant the lock protects, in words, before the move.
• Move the state and its lock together, into the module that owns them. State in one module and its lock in another is a bug waiting for load.
• If two areas share one lock, that is the coupling — resolve it before splitting, not during.

References — load on demand

• references/refactoring-patterns/smell-catalog.md — the full catalogue with triggers
• references/refactoring-patterns/refactoring-workflow.md — safe workflow and rollback
• references/refactoring-patterns/composing-methods.md — extract, inline, replace temp
• references/refactoring-patterns/moving-features.md — move method/field, hide delegate
• references/refactoring-patterns/organizing-data.md — replace primitive, encapsulate
• references/refactoring-patterns/simplifying-conditionals.md — guard clauses, polymorphism
• *-original.md — the source skill's own framework prose, kept verbatim

Gotchas

• The big-bang restructure. A branch that reorganises everything cannot be reviewed, cannot be bisected, and conflicts with every parallel session. Small steps on main beat a heroic branch, every time.
• Refactoring under a shared file. If other sessions or teammates are editing the same file right now, the cut is a coordinated operation, not a background tidy. Check first; conflicts here lose work rather than just time.
• "Modular" that still imports the old module. If every new module imports the file you split, you moved text, not boundaries. Constants and shared state first — that is what step 1 and 2 are for.
• Renaming during a move. Move, commit, then rename. A diff that does both is unreviewable and hides accidental behaviour changes.
• Treating traffic as universal proof. Traffic/canary proof is meaningful for a live served boundary, not an offline or sealed module. Match the consumer or integration receipt to the boundary, while retaining its deletion authority and migration check.

Troubleshooting