Smells & Refactoring

Feathers defines legacy code as code without tests, regardless of its age or quality. The dilemma: to change code safely you want tests first, but to make the code testable you usually have to change it first.

Refactoring changes how code is structured, never what it does. Callers see the same observable results before and after. Adding a feature, fixing a bug, or changing output is not refactoring — it is a behavior change, done separately.

Take a chunk of code inside a function, move it into a new function whose name says what it does, and call that new function from the original spot. Behaviour stays identical; only the structure changes.

Red: write one small failing test for behavior you want. Green: write the simplest code that makes it pass. Refactor: clean up the code and the test while all tests stay passing. Then repeat with the next small behavior.

A code smell is a surface hint that code may need restructuring — it is not a bug. Duplicated Code = the same logic in several places. Long Method = one function doing too much. Long Parameter List = a call taking too many arguments.

It compares design shortcuts to borrowing money. The principal is the one-time effort to fix the shortcut properly; the interest is the extra effort every future change costs while the shortcut remains. Shipping fast borrows; refactoring repays.

Fix 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.

A characterization test pins down what code actually does today, not what it should do. You call the code, assert a deliberately wrong value, read the failure message to learn the real value, and change the assertion to match. If the real behaviour looks like a bug, you still record it, then ask before changing it.

Guard clauses flatten deeply nested if/else by returning early for the unusual or invalid cases first, leaving the normal path unindented at the end. Decompose Conditional instead replaces a complicated condition and its branch bodies with well-named function calls.

Feature Envy: a function is more interested in another object's data than its own — it keeps calling that object's getters. Inappropriate Intimacy: two classes know too much about each other's internals. Fix by moving behaviour to the data (Move Function) or separating the classes.

Fowler classifies debt on two axes: was it chosen on purpose (deliberate) or discovered later (inadvertent), and was it a sensible call (prudent) or careless (reckless). The four combinations need very different responses.

Incremental cleanup works when the architecture is sound and the pain is spread across code you touch often. You need a bigger planned effort when the problem is structural — wrong boundaries, dead platform — so no local edit can reach it.

A seam is a place where you can change what code does without editing that code in that place. Its enabling point is where you make the choice (a constructor argument, a build configuration, a linker path). Seams let you swap real collaborators for fakes so untestable code becomes testable.

Don't refactor blind. First pin the current behavior with characterization tests — tests that record what the code actually does today, bugs included. To get the code under test, introduce seams: small, low-risk points where you can substitute a dependency. Then refactor in small steps, running the tests each time.

Divergent Change: one module keeps changing for many unrelated reasons — it does too much. Shotgun Surgery: one change forces small edits across many modules — a responsibility is scattered. They are opposites: split the module, or gather the scattered pieces.

Small debt: clean up as you pass through the code, leaving it better than you found it. Medium debt: replace it piece by piece behind a stable interface while the system keeps running. Full rewrites are a last resort - slow, risky, and they freeze delivery.

The idea that visible neglect invites more neglect: one unfixed mess signals that mess is acceptable, so people stop caring and decay accelerates. Signals include ignored warnings, disabled tests, copy-pasted hacks and TODOs nobody ever removes.

All four let you add new, tested code without untangling the old code. Sprout puts the new logic in a new method or class and calls it from one place in the old code. Wrap renames the old method and puts a new method in its place that calls both, or puts the old object behind a decorator that adds behaviour.

When the same group of arguments keeps travelling together through many functions, bundle them into one small object and pass that instead. The parameter lists shrink and the group finally has a name.

Replace a local variable holding the result of an expression with a small function that computes that expression, then call the function wherever the variable was used. Fewer locals makes the surrounding code easier to extract into other functions.

You wear one of two hats: adding functionality (new tests, new behavior) or refactoring (structure only, no behavior change, no new tests). Swap deliberately, never wear both at once. If a refactoring uncovers a bug, note it, finish or revert the structural step, then switch to the functionality hat to fix it.

Data Clumps: the same group of values keeps travelling together (e.g. street, city, zip). Primitive Obsession: using built-in types like string or int for domain ideas such as money or email. Both are fixed by creating a small named type.

During an urgent production hotfix, in code about to be deleted or replaced, in files with big in-flight branches, in frozen or audited code, in areas you don't understand, and when the "cleanup" would actually change behaviour.

The algorithm is: identify change points, find test points, break dependencies, write tests, then make your change and refactor. To find test points you sketch which values a change can affect, then look for a narrow place all those effects flow through — a pinch point — and test there.

When the same switch on a type code appears in several places, create one subclass (or strategy object) per case, move each branch's body into the matching subclass, and let the language dispatch. Adding a new case then means adding a class instead of editing every switch.

Prefer incremental refactoring: it keeps the system shippable and delivers value continuously. Rewrite only when the platform itself is untenable. Strangler fig routes traffic feature-by-feature from old to new until the old is unused; branch by abstraction puts an interface in front of a component so old and new implementations can coexist behind a toggle.

A message chain is code like a.getB().getC().getD() — the caller navigates a long path through objects. It breaks the Law of Demeter ("only talk to your immediate friends"). Hiding each hop behind a delegating method can leave classes that only forward calls: Middle Man.

Write debt down where the team and stakeholders can see it, with the fix cost and the pain it causes. Then fix first the items in code you change often, because those cost you repeatedly.

Fold most cleanup into feature estimates instead of asking permission for it, keep a small explicit allocation for larger items, and argue in terms of delivery speed and risk rather than code beauty.