Clean code and refactoring

Code is read far more often than it is written, and by people who were not there when it was. "Clean" is not an aesthetic; it is a measure of how much a reader has to hold in their head to change something safely. Several lessons in this curriculum have already covered pieces of it — naming and method size in the methods lesson, access and packages in the conventions lesson, exceptions in their own course. This one is about the rest: duplication, the smells that predict bugs, the refactoring moves that remove them, and technical debt as a decision rather than a lament.

#The measure: what the next reader has to know

Every shortcut has the same shape. It saves the writer a minute and costs every subsequent reader a minute, and there are more readers than writers. A method the reader must run in their head to know what it returns, a variable whose meaning depends on where in the method you are, a boolean argument whose meaning is at the call site's other end — each is a small tax, and a service is thousands of them.

So the test for any of the rules below is not "is this elegant" but "can a competent colleague change this correctly without asking me". If they cannot, something on this page is the reason.

#Duplication, and the rule of three

Duplicated code is the smell everyone knows and the one most often fixed wrongly. Two copies of the same six lines are a maintenance risk: a bug fixed in one and not the other. But the fix — extract it — creates an abstraction, and an abstraction built from two examples is usually wrong: the two cases were similar by coincidence, the third case does not fit, and the shared method grows a boolean parameter, then two, then a switch on which caller it is serving.

The rule of three: tolerate the second copy, extract at the third, because three cases show you what actually varies. And the corollary from Sandi Metz: duplication is far cheaper than the wrong abstraction. Un-extracting a bad shared method — inlining it back into its callers and removing the parameters that selected behaviour — is a legitimate refactoring, and often the right one.

The exception is knowledge, not code. A tax rate, a validation rule, a URL: those must exist once, whatever the code around them looks like. DRY was always about knowledge; it was applied to text.

#Smells: what predicts a bug

A smell is a surface feature that correlates with a deeper problem. None is a rule; each is a reason to look.

Two of them deserve a sentence. Primitive obsession is the one with the largest payoff per fix: charge(Money amount, CustomerId to) cannot have its arguments swapped, where charge(long, long) can and eventually will; records make the wrapper a one-line declaration. Flag arguments are the cheapest to fix — sendNow(order) and sendLater(order) — and the most common in code reviews.

#The moves: refactoring is a vocabulary

A refactoring is a change that preserves behaviour, small enough to be obviously correct, done with the tests green before and after. The named ones matter because a name is a plan. The six that do most of the work:

• Extract method. A block with a comment above it is a method whose name is the comment. The methods lesson's inlining budget agrees: a small named method costs nothing at run time.
• Inline method. The opposite, for a method whose body is as clear as its name — and for un-extracting the wrong abstraction.
• Introduce parameter object. The long parameter list becomes a record; the record then attracts the behaviour that belongs with it, which is how value objects are born.
• Replace conditional with polymorphism. The growing switch on a type becomes strategy or state, from the patterns lessons. The trigger is the third case.
• Move method. Feature envy's fix: the method goes where the data is.
• Replace magic number with constant — and then, usually, with a type: Duration.ofSeconds(30) says what 30000 did not.

Before and after, in one example:

double total(List<Item> items, boolean express) {
    double t = 0;
    for (Item i : items) t += i.price * i.qty;             // sum
    if (t > 500) t = t * 0.95;                              // bulk discount
    if (express) t += 50; else t += 20;                     // shipping
    return t;
}

Money total(List<Item> items, Shipping shipping) {
    Money subtotal = subtotal(items);
    return applyBulkDiscount(subtotal).plus(shipping.cost());
}

Nothing about the business changed; the reader now has four names where there were three comments, a Shipping type instead of a boolean, and a Money that cannot be accidentally added to a quantity. Each step was one move, run with the tests between.

The discipline is the part people skip: refactor or change behaviour, never both in one commit. A commit that moves code and fixes a bug is a commit nobody can review, and a bug in it is a bug nobody can bisect.

#Comments

The conventions lesson said it: comments explain why, not what. A comment that describes what the code does is a name the code should have had; a comment that says why the code is this way — the bug it works around, the requirement nobody would guess, the alternative that was tried and failed — is the most valuable line in the file, because it is the one thing the code cannot say. // see PAY-1432: the gateway rejects amounts over 1,000,000 paise in one call earns its place. // loop over the orders does not.

Commented-out code is the special case: delete it. The version history has it, and every reader who meets it has to decide whether it matters.

#Technical debt is a decision

Ward Cunningham's metaphor was precise and it has been flattened. Debt is borrowing: you ship a design you know is not right, because shipping now is worth more than the design, and you pay interest — every change in that area costs more than it should — until you repay the principal by fixing it. Three things follow from taking the metaphor seriously:

• Debt taken deliberately is fine. A hard-coded rate to make the launch, with a ticket to make it configurable, is a good loan. The problem is debt taken without noticing, which is most of it.
• Interest, not principal, is what to measure. A bad design nobody touches costs nothing; leave it. A bad design in the file that changes every week is the most expensive thing you own; fix it, and the argument for fixing it is the number of changes, which you can count.
• Write it down. A debt register — a file, a label, a list — turns "the code is bad" into "these six things, each with a cost and a trigger". That is a conversation an engineering manager can have; "we need a refactoring sprint" is not.

The quadrant Martin Fowler drew is worth keeping: debt is deliberate or inadvertent, and prudent or reckless. Deliberate and prudent is engineering; inadvertent and prudent is learning ("now we know how we should have done it"); reckless in either form is the kind that sinks a codebase, and the smells table above is how it looks up close.

#In review

A reviewer's checklist, from this page and the ones it points to:

1. Could I change this without asking the author? If not, what would I need to know, and can the code say it?
2. Is anything here the second copy of something? The third?
3. Any flag arguments, long parameter lists, or primitives standing in for a type?
4. Does any comment describe what instead of why? Any code commented out?
5. Does the commit refactor and change behaviour?
6. If this is debt, is it deliberate, and is it written down?