Refactoring is a wonderful practice for making code and change easier to manage. That said, here are some reasons why we should not proceed with a refactor.
1. The change is not really a refactor
Many people use the word “refactoring” incorrectly. If we’re embarking on a change that is not really refactoring (for example looking at a bug or an adjustment after a third party change), we can’t fix it with refactoring.
What to do instead: we need to think and talk about it differently from refactoring. We can stop and consider how the system will change (from what to what) and raise it with teammates to discuss why it matters, what action to take, and when.
2. There are examples that don’t fit our planned refactor
We often reach a point during refactoring where it seems like there is an easy improvement that applies to almost all cases. It’s usually better not to impose additional abstraction if it only matches “almost” all cases.
Here’s an example inspired by the Noisy Animals kata. Consider this code:
def make_noise(loud: true)
noise = {
'leopard' => { quiet: 'growl', loud: ['growl', 'growl'] },
'owl' => { quiet: 'hoot', loud: ['hoot', 'hoot'] },
'eagle' => { quiet: 'caw', loud: ['caw', 'caw'] },
'mouse' => { quiet: "\n", loud: ["\n"] },
'snake' => { quiet: 'slither', loud: ['hiss'] },
}.fetch(species)
puts noise.fetch(loud ? :loud : :quiet)
end
Those repeated strings on some lines might be irritating, but two lines don’t share the repetition. We’re better off leaving the code as is than trying something like this:
def make_noise(loud: true)
noise = {
'leopard' => 'growl'
'owl' => 'hoot',
'eagle' => 'caw',
'mouse' => "\n",
'snake' => 'slither'
}[species]
if loud && species == 'snake'
puts 'hiss'
elsif loud && species == 'mouse'
puts "\n"
elsif loud
puts noise, noise
else
puts noise
end
end
Here we’ve moved from a single ternary conditional to an if statement with four branches. Knowledge of how each species behaves was contained in a single line but is now split up over the entire method. The method is also almost double its original length.
What to do instead: perhaps there is a different code smell we can investigate, or maybe we need to stop where we are until a new requirement demonstrates the need to change.
3. Another refactor is already in progress
We’re often confronted with more than one code smell at a time. Perhaps a piece of code seems like it should be extracted to a method, and inside that code is an unnecessary local variable. Create the method and replace all occurrences of the original code with the method call before you remove the local variable. Alternatively, remove the variable before you extract the code into a method. After you finish one, save, run the tests, and commit, all before you move on to the next.
Whatever you do, don’t try to do both at the same time. Doing them separately may seem like a hassle, but the risk of a typo and the benefit of knowing that you started each change from the safety of green tests are worth much more than the hassle (and there are techniques you can learn to reduce the hassle).
What to do instead: live with the code that isn’t ideal long enough to see the results of your current refactor. The current refactor may help you realise something new about the code that you would have missed if you tried to do both at the same time.
4. The code is unlikely to change
There’s no benefit to improving code that never changes, even if it is highly complex. This is the basis of Michael Feather’s process for identifying good refactoring targets (spoiler: code that is complex and changes often is the best place to look).
We should be aware in this case that it is really easy to apply wishful thinking: we imagine that the code that everyone is scared of will never change, simply because we cannot think of how to improve it.
What to do instead: find something else that is more likely to change to refactor, or pause refactoring and move on to the next requirement.
5. There are no tests
If we’re changing tests (or we have none), we have no guarantee that behaviour is unchanged. If we are possibly changing behaviour, we are not refactoring.
Here’s a delightful extract from Refactoring Javascript:
HOW CONVERSATIONS ABOUT REFACTORING SHOULD GO UNTIL TESTS ARE WRITTEN
“I refactored login to take email address and username.”
“No, you didn’t.”
“I’m refactoring the code to ____”
“No, you aren’t.”
“Before we can add tests, we need to refactor.”
“No.”
“Refactoring th–”
“No.”
“Refa–”
“No.”
What to do instead: Instead of “refactoring” without tests, we can start by writing some tests! Writing new tests will help us learn about the code. After that, we can come back to refactoring.
6. Refactoring reveals more risk than imagined
Sometimes we tidy the code to a certain point and realise that there is something we don’t understand. That’s ok, we can stop where we are now. If we’ve been working in small low-risk steps the way that the refactoring community encourages, the tidying we did already was either valuable and should be kept, or is really safe and easy to revert.
From Martin Fowler’s description of the Refactoring book: “its essence is a series of small behavior-preserving transformations… You also avoid having the system broken while you are carrying out the restructuring - which allows you to gradually refactor a system over an extended period of time.”
What to do instead: Simply pause where we are and continue another time when we know more. That “more” may be something about our product that reveals that some code is overly complex or unnecessary. It might also be a new requirement that shows us a new way that the code is likely to change in future.
This might lead to more refactoring
We’ve discussed several reasons not to refactor. Ironically, we might end up doing more refactoring as a result, because we won’t be trying to use refactoring to solve unsuitable problems. The refactoring we do will be faster, have a bigger impact, and be more rewarding.