Jake Worth

• refactoring

Refactoring (noun): a change made to the internal structure of software to make it easier to understand and cheaper to modify without changing its observable behavior. —Martin Fowler

“Easier to understand and cheaper to modify”: those both sound pretty good! Shouldn’t we be refactoring all of the time? It seems like practice with no tradeoff.

When I review code, I often request that changes be removed. Even when I think the changes are are objective improvements, even when they support my personal preferences.

My reasoning? Refactors are not free. In this post, I’ll list a few times when I think a refactor is worse than leaving the code unchanged.

No Automated Tests

Do you have automated tests? Can you run them? Do you trust them? If you don’t, don’t refactor.

A mentor once told me: “refactoring without tests is not a thing.” In other words, refactoring without tests is not refactoring, because refactoring preserves the current behavior, and without good automated tests, you have no way to prove you’re doing that. Instead, you’re taking something that works and rolling the dice until it breaks.

A type system, or a pre-compiled language, will catch some of the mistakes you’ll make while coding. But in a dynamic language like Ruby, without a robust test suite, you’re going to break something.

Refactoring without tests is a risk.

It’s the Only Change

Is your refactor the only change to a file? If so, probably don’t refactor.

You may find yourself reading a file, start refactoring it, and then realize you don’t need to change it. Should you commit the refactor anyway? Probably not.

When I read a commit message that says “Fixes profile page crashing”, I expect an atomic commit, where everything in that commit supports the goal. Only by closely reading the Git diff can I learn that isn’t true. It’s just easier to assume that everything matters.

It’s Arbitrary

Avoid arbitrary refactors; they waste time and make important changes harder to find.

Here are some changes that can improve the code, but aren’t defensible by themselves:

• ❌ Adding or removing whitespace
• ❌ Breaking up long lines
• ❌ Trivial variable renames (e to event)
• ❌ Personal preferences that are not enforced with tooling or conventional to the language or framework (converting JavaScript functions to arrow functions)

Why are these bad? For one, you’re stepping on the Git history. Many developers use Git to understand past decisions. Touch a line of code, and you bury that context under a new, surface-level commit. For most developers I’ve met, that puts the context out of reach.

Also, it’s frustrating to review pull requests with changes like these. I’ve reviewed pull requests where at the end of a long, close read you say “this changes the Jest config, and then removes semicolons from file in the ‘types’ and ‘api’ directories.” Excellent programmers make every change count and avoid random, arbitrary contributions.

Okay Refactors

So what are okay refactors? If all of these are true, you might be on the right track:

• ✅ The file must be changed to improve the user (great!) or developer (good!) experience
• ✅ You have some way to prove you’re not breaking something: test coverage, type safety, etc. Or the change is so simple that you can’t mess it up (consider this a very high bar)
• ✅ The refactor improves the code. This can be subjective, but there are good and bad examples

If you meet all these criteria, go ahead and commit your refactor. One exception to this are tests; refactor them as much as you want.

Conclusion

In conclusion, I discourage refactors when:

• There is no way to prove behavior isn’t changing
• It’s the only change
• It’s an arbitrary change

Otherwise, refactor away.

What are your thoughts on this? Let me know!