Working with Legacy Code

## The definition Michael Feathers, in *Working Effectively with Legacy Code* (2004), proposes a deliberately provocative definition: > **Legacy code is code without tests.** This is not the colloquial meaning. Colloquially "legacy" means old, inherited, written in an unfashionable language, or written by someone who has left. Feathers discards all of that because none of it determines whether you can change the code with confidence. What determines that is whether you have a **fast, deterministic, automated check** that tells you, within seconds, that behaviour you did not intend to change is still intact. Terms used above: - **Test** here means an automated unit-level test: it runs without a database, network, filesystem, container, or human, and it fails loudly when behaviour changes. - **Behaviour** is what the code does that someone can observe: return values, exceptions thrown, calls made to collaborators, state mutated. ## Why the definition is useful It converts a vague complaint ("this codebase is horrible") into an actionable engineering condition ("this code has no feedback loop"). Two consequences follow: 1. **Editing becomes fear-driven.** Without tests, the cheapest way to add a feature is to *not touch* the existing code: copy a method and tweak the copy, add another `if`, add another flag. Each such act increases duplication and coupling. Feathers calls this the downward spiral of legacy code: fear causes changes that make future change scarier. 2. **It sets a clear success criterion for improvement work.** You are done making a piece of code non-legacy when you can exercise it in a test harness quickly and independently. ## The Legacy Code Dilemma Feathers states it directly: > **When we change code, we should have tests in place. To put tests in place, we often have to change code.** The circularity is real. Typical blockers you hit when you try to instantiate a class in a test: - The constructor reaches out and creates a database connection, opens a socket, or reads a config file. - The class calls a static/global singleton (`Registry.getInstance()`, `DateTime.now()`, `System.getEnv()`). - The method you care about is buried in a 900-line method with no return value, whose only effect is a side effect on a global. - Constructing the object requires constructing a large graph of other objects ("irritating parameter" / "construction blob" problems). ## How the dilemma is resolved The resolution is not "give up and test through the UI". It is: make the **first** change one whose risk you can control by other means, so you spend risk only once. - **Prefer mechanically safe edits.** IDE-verified automated refactorings (Extract Method, Introduce Parameter, Rename) are far safer than hand editing, because a tool performs them. Feathers' rule: while you have no tests, only perform edits you can argue are behaviour preserving by construction. - **Preserve signatures.** When breaking a dependency, move whole blocks of code without retyping them and keep parameter lists identical, so you cannot silently reorder or drop an argument. - **Exploit an existing seam** (a place where you can change behaviour without editing in that place) if one already exists, e.g. the class already takes a collaborator as a constructor argument. - **Sensing vs separation.** Sometimes you do not need to isolate the class fully; you only need to *sense* what it did (capture a value it would have written out). That is often a smaller change than full decoupling. - **Then write characterization tests** that pin down current behaviour (bugs included), and only then refactor or add features. ## Common edge cases - **Code with tests that are slow or non-deterministic** is, for practical purposes, still legacy: a suite that takes 40 minutes or fails randomly gives no usable feedback, so people stop trusting it. - **Code with tests that assert nothing** ("it ran without throwing") is legacy too. - **Generated code** is usually exempt: you test the generator, not the output. - **Code you are about to delete** should not be tested; confirm it is dead and remove it. ## The pragmatic scoping rule You do not test an entire legacy system before touching it. You test **around the change point**, in what Feathers' Legacy Code Change Algorithm calls the test-covering step. Coverage grows where the work happens; untouched regions stay untested until someone needs to touch them.