Last week was about Jakub, the team lead who keeps saying “not now, we can’t afford the risk”. We looked at the three reasons behind that no, and how to answer each one in the currency he’s measured in. It was a post about winning an argument.
This week is about not having it.
Something gets lost when refactoring becomes a negotiation. In my experience, most of what I want to change on any given day was never Jakub’s call to make. The state machine copied into six services, yes, that one needs a conversation. But the confusing variable name, the twelve-line method doing three things, the conditional nobody can read out loud? Those fit inside the branch we already have open. Nobody approves them, because nobody was ever going to be asked.
Two Kinds of Refactoring, One Word
The word “refactoring” covers two jobs with almost nothing in common, and collapsing them is what makes every conversation about it go badly.
The first kind changes a shape that other code depends on. Extracting a state machine out of six services, splitting a module, moving a boundary. It touches files owned by other people, it can’t be finished in an afternoon, and if it goes wrong it goes wrong in production.
That work needs a plan, a slice sequence, and a conversation with Jakub. Most of last week’s post was about that conversation.
The second kind never leaves the file we’re already editing. Renaming a local variable, extracting a private method, flattening a conditional, deleting a comment that lies. The blast radius is the function we already have open, and the cost is measured in minutes. Nobody schedules this. By the time a ticket existed for it, the work would have taken longer to describe than to do.
When we ask for permission to refactor, we are almost always asking about the first kind while thinking about the second. Jakub hears “reach into working code and hope”, because that’s what the word means on his side of the table. We meant “rename d to daysSinceSet while we’re in here anyway”. No wonder the answer keeps being no.
The Boy Scout Rule Has a Missing Half
Robert C. Martin’s Boy Scout Rule, leave the code better than you found it, is the standard advice here1. It’s right, and it mostly doesn’t work. Not because the rule is wrong, but because it’s incomplete in two ways.
The first is cadence. The rule has to run on every branch, not on the ones where we happen to feel energetic. Tidying only fires when we’re already in a file, so the files we open most get cleaned most, which is exactly the right distribution. Do it sporadically and nothing compounds, because no file gets touched twice before we forget. The second gap only shows up when we hand the work to someone else.
Let’s say we set routes at a bouldering gym. The system tracks each boulder problem: when it went up, how many climbers have topped it, how many have flagged it as sandbagged. A scheduler decides which routes are stale enough to strip and reset. This week’s feature request is that competition routes get frozen until the competition is over, so nobody resets the finals boulders the night before.
We open RouteResetScheduler and find this:
private boolean check(Route r) {
long d = DAYS.between(r.setOn(), LocalDate.now());
if (d > 42 || (d > 28 && r.sends() > 60) || (d > 21 && r.flags() > 3)) {
return true;
}
return false;
}Five magic numbers, a method named check, a variable named d, a condition that takes a second read to parse, and an if/return true that could have been a return2. It works. It has worked for two years. And now we have to add a fourth rule to it.
The Boy Scout Rule says clean it up while we’re here. So we do, in the same commit as the competition feature. Then the pull request lands on someone’s desk showing forty changed lines, and the reviewer has to work out which of them changed behavior. Their only safe move is to read all forty as if each one might be a bug. That’s slow, the review hangs for two days, and next time we remember those two days.
That’s the missing half. The rule tells us to leave the code better. It doesn’t tell us how to hand the result to another human in a form they can check quickly. Without that second part, the Boy Scout Rule mostly teaches us that tidying makes reviews worse, and the lesson we learn is to stop doing it.
Separate the Commits, Not the Branches
The fix is small enough to feel like it can’t be the answer. Keep the refactoring in the same branch, but never in the same commit as the feature.
Same branch, because a separate branch means a separate review, a separate merge, and a separate argument about priority. Separate commits, because a commit is the unit a reviewer reads and the unit git revert operates on3.
Here’s the route scheduler again, done as four commits instead of one. First, the names, and nothing else:
// commit 1: rename only. No behavior change.
private boolean isDueForReset(Route route) {
long daysSinceSet = DAYS.between(route.setOn(), LocalDate.now());
if (daysSinceSet > 42
|| (daysSinceSet > 28 && route.sends() > 60)
|| (daysSinceSet > 21 && route.flags() > 3)) {
return true;
}
return false;
}Then the condition gets names for its three cases, and the if/return true collapses into the expression it always was:
// commit 2: extract the three reset triggers. Still no behavior change.
private boolean isDueForReset(Route route) {
long daysSinceSet = DAYS.between(route.setOn(), LocalDate.now());
return pastMaximumLifespan(daysSinceSet)
|| wornOutByTraffic(daysSinceSet, route.sends())
|| rejectedByClimbers(daysSinceSet, route.flags());
}
private boolean pastMaximumLifespan(long daysSinceSet) {
return daysSinceSet > 42;
}
private boolean wornOutByTraffic(long daysSinceSet, int sends) {
return daysSinceSet > 28 && sends > 60;
}
private boolean rejectedByClimbers(long daysSinceSet, int flags) {
return daysSinceSet > 21 && flags > 3;
}The helpers take the values they need rather than reaching back into Route for them, which keeps them honest. Each one can be read, and tested, without knowing what a Route is.
That extraction has a precondition worth saying out loud, the kind of thing that turns a “no behavior change” commit into a lie. The original only evaluated r.sends() when the day check passed. The new version evaluates it whenever the first trigger comes back false. Here that’s invisible, since sends() and flags() are plain accessors. If either lazily loaded from the database or wrote a metric, the guard would have to stay inside the helper.
Commit 3 promotes those five magic numbers to named constants4. Only then does commit 4 add the feature we were actually sent here to build:
// commit 4: the competition freeze. The only commit that changes behavior.
private boolean isDueForReset(Route route) {
if (route.isFrozenForCompetition()) {
return false;
}
long daysSinceSet = DAYS.between(route.setOn(), LocalDate.now());
return pastMaximumLifespan(daysSinceSet)
|| wornOutByTraffic(daysSinceSet, route.sends())
|| rejectedByClimbers(daysSinceSet, route.flags());
}Note where the competition rule went. Not route.zone().equals("comp") in the scheduler, but route.isFrozenForCompetition(), a question the route answers about itself, where the knowledge belongs5. The refactoring didn’t just make room for the feature. It made the feature’s right home obvious.
What the Split Buys
Four commits instead of one sounds like more work for the reviewer. It’s the opposite, and the reason is precise.
The reviewer opens commit 4 first, because that’s the one with the feature in it. Nine lines and one new guard clause. They can hold it in their head at once, decide whether the competition freeze is right, and be done. Commits 1 to 3 get a different read: skim, confirm no behavior moved, approve. A reviewer who can trust the commit boundaries only has to think hard about one commit out of four.
That trust is also what makes the tidying safe to keep. If the competition freeze turns out to be wrong, git revert on commit 4 takes it out and leaves the cleanup in place. Under a single mixed commit, reverting the bug also reverts the rename, the extraction, and the constants. An incident at 11 PM throws away three days of tidying, and whoever runs the revert never sees what they deleted.
There’s a version of this that fails. If the “no behavior change” commits quietly change behavior, the whole thing inverts. The reviewer trusted the label, skimmed, and missed a bug. So the boundary is a promise, and the way we keep it is boring. Run the tests between each commit. If a rename commit needs a test change, it wasn’t a rename6.
That discipline assumes tests are worth running, which is not safe to assume in the code most in need of tidying. Where the suite is thin, the safe subset shrinks to what an IDE can perform and verify for us. Rename, extract method, inline variable. Anything past that wants a characterization test7 first, and writing one is itself the small, self-contained work this post is about.
Two things weaken the split, and both are common. Squash-merging collapses the four commits into one on the main branch, so the “revert commit 4” move is gone. And most review tools open the whole-branch diff by default, so the reviewer has to choose the per-commit view to get any benefit at all.
Neither kills the practice. The per-commit view is one click, and the team should make it the default. Even under a squash, the discipline still keeps us honest while we work. But we should know which of the two payoffs our own setup gives us.
Where the Line Actually Is
I’ve been drawing a clean line between the small work and the work that needs a conversation, and clean lines deserve suspicion. So here is where I actually draw it, and where I get it wrong.
Here is the test I use. Would this change show up in someone else’s pull request as a conflict, or in someone else’s file as a surprise?
A private method extracted inside a class we’re already editing, no. Renaming a public method that six services call, yes, and that’s why the scheduler’s method above is private. Turning its three predicates into a RouteResetPolicy that has to be threaded through the mobile app, the wall display, and the setter’s iPad, definitely yes.
That last one is a real improvement, and it’s exactly the kind of thing last week’s post was about pricing and slicing.
The failure I keep making is the second commit, the one that looks as small as the first. The rename is genuinely two minutes. The extraction is another two. Then extracting suggests the class boundary is wrong, and forty minutes later the feature hasn’t been started and the diff is unreviewable.
There’s no rule that catches this, only a habit. When the tidying stops being obvious, stop. Write down what we found, and finish the feature. That note is what makes the eventual conversation with Jakub a good one, because it arrives with a concrete example already in it.
The other limit is that this doesn’t reach code we never open. The worst module in the system, the one nobody has touched in three years and that I am fairly sure I wrote, stays exactly as bad as it is. Perhaps that’s fine. If nobody opens it, it costs nobody anything. But we should not tell ourselves that daily tidying is a strategy for the parts of the codebase we’re avoiding.
Track What It Bought
The small refactoring never needed the argument with Jakub. The useful by-product is that it builds the case for the argument we still have to have, and that case is worth collecting on purpose.
One of last week’s three reasons was that refactoring loses because it shows up in adjectives while everything else shows up in numbers. Daily tidying has the same problem, with one advantage. We’re already generating the data, and nobody has to approve collecting it.
The measurement is a comparison, not an absolute. Pick a handful of areas we’ve been cleaning, and a handful we haven’t, and track how long features take in each. Not story points. Measure time spent from starting the branch to merging it.
The obvious objection is that the two areas differ for reasons that have nothing to do with tidying: age, coupling, domain, who works in them. The confound is real, and it’s why one reading proves nothing. What survives it is the standard the series has used since The Cost of “Just Ship It”. A trend, over a quarter, in areas of comparable size, or better still the same area before and after. It’s directional evidence about our own code, which beats an industry study about somebody else’s.
After a quarter, we get to say something like this:
The last four features in the scheduler averaged two days. The same size of change in the billing module averaged six. The scheduler is the one we’ve been tidying as we go.
Those numbers are made up, but the real point is the message. Notice what a sentence like that does. It doesn’t ask for a refactoring sprint. It shows Jakub that the practice already paid, on his scoreboard, without costing him a single planned day. The next conversation about the big refactor starts from evidence rather than from adjectives, and it starts with him already knowing the tidying works.
The Takeaway
Most refactoring doesn’t need permission, because most refactoring never leaves the file we already have open. What it needs is a commit discipline: same branch, separate commits, tests green in between. That turns tidying from a thing that makes reviews slower into a thing that makes them faster. It also builds the evidence for the one refactor that does need Jakub’s yes.
TLDR
Most refactoring doesn’t need a dedicated sprint. It belongs inside the feature branch we already have open.
The Boy Scout Rule works when it runs on every branch, not sporadically. Its missing half is a commit discipline.
The technique: refactoring commits and feature commits stay separate, in the same branch. Review gets faster and
revertstays safe. Squash-merging costs us the revert payoff, so know which one our setup gives us.The line is blast radius, not size. If the change surfaces in someone else’s file, it needs a conversation.
Track time-to-implement in areas we’ve tidied against areas we haven’t. One reading proves nothing. A quarter of them is the evidence for the next conversation with our lead.
Try This Week
In the branch currently open, before writing any new code, make one refactoring commit. Rename a variable that made us pause. Extract a method from the middle of a long one. Flatten a conditional. One small thing, committed on its own, with the tests green before and after.
Then look at the commit. That is the entire practice, and it needed nobody’s approval. Do it on the next branch too, and the one after. In three months there will be files that are nicer to work in, and the start of a trend line to point at.
Media attributions:
Cover image by the author (generated with Gemini)
Martin's essay The Boy Scout Rule is in 97 Things Every Programmer Should Know (O'Reilly, 2010), and the same idea runs through Clean Code. The wording varies by retelling. His own version is about campgrounds, not codebases.
if (condition) return true; return false; is its own small genre. It survives in codebases forever because it is never quite annoying enough to fix and never quite readable enough to leave alone.
The Single Responsibility Principle, applied to history. Same author as the rule this section opened with. Conventional Commits makes the split readable (for both humans and machines): the prefix classifies the commit, and that classification tells a reviewer what kind of reading it needs. The real value shows up when we cannot pick a prefix, because a commit that refuses to classify is one that did two things.
MAX_DAYS_ON_THE_WALL took the team longer to agree on than the refactoring did. This is normal and I have stopped fighting it because I finally accepted there are two unsolved things in programming:
Naming things
Cache invalidation
Off-by-one errors
This is the same move as Why Your Service Class Knows Too Much, at one twentieth the scale. Behavior goes to the object that owns the data it needs. The scale changes, the rule doesn't.
The one real exception is a test that referenced the old name. If the change is the name and nothing else, it's still a rename. If the assertion changed, we've been refactoring under a flag of convenience.
Michael Feathers' term, from Working Effectively with Legacy Code. A characterization test pins down what the code does now, not what it should do. We are not asserting it is correct, only noticing when its behavior changes.