Reasons Not to Refactor

> What struck me was how often the core problems were architectural

Agreed - I remember at a previous job a new guy turned up and directed a rewrite of an old Perl application into a new Go version. He crowed on and on about how Go was so much faster than Perl but the underlying issue was the original architecture (old one was pinned to a single core and polled a database once a minute, the new one was dispatching requests pulled off a Kafka queue). The old one had been written when the system was handling less than a transaction a minute and the only attempts to fix it were upgrading Perl or various libraries.

I'd argue with that. Small-scale "micro-refactoring" operations are safe almost by definition. [1]

It depends on your language. In a large Python system [2], all bets are off, but in Java if you use your IDE to rename a method to have a clearer name or change the signature of a method or extract or inline a method the risk of breaking anything is close to zero whether or not you have tests.

Personally I think there is no conflict between feature work and micro-refactorings. If micro-refactorings are helpful for a feature you're working on, jump to it!

Personally I've had the experience of being a Cassandra [3] when it comes to YAGNI; [4] maybe it is different for a junior dev but so often I go to a meeting and say "what if you decide you need to collect N phone numbers instead of 2?" and over the next few months there is a stream of tickets for 3, 4, 5 phone numbers until they finally make it N. Or a problem w/ the login process that is obvious to me becomes the subject of a panic two months later.

As such I see the rule of three [5] to apply DRY is too conservative, it is way two common that the senior dev wrote two cases and then the junior dev comes in and copies it 15 times. At least on the projects I've worked on (mainly web-oriented, but many involving 'intelligent systems', data science, etc.) people have made way too many excuses for why repeating themselves is good and it has had awful consequences.

[1] https://en.wikipedia.org/wiki/Code_refactoring

[2] the best case for Python is that a large system in some other language could be a small system in Python

[3] https://en.wikipedia.org/wiki/Cassandra

[4] https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

[5] https://en.wikipedia.org/wiki/Rule_of_three_(computer_progra...

I'd extend #2 to any change, not just non-trivial. It's the classic Kent Beck tweet, "for any change, make the change easy (warning: this may be hard), then make the easy change"

Fixing my mental model of thinking about refactoring as a separate thing from "normal" development was key for me. Once I viewed refactoring as a thing that you do all the time as part of development, then I stopped even asking this question.

> More importantly, no changes over the last few years means that absolutely no one in the company has in-depth and current knowledge of how that code works, so a refactor is pointless because no one knows what the specific problems actually are. reply

That’s is the actual reason you might need to rewrite it. If your critical system is written in an ancient script that nobody can understand, and does no longer supported, and as a security risk, at some point it will simply stop working. And there will be nobody that can fix it.

Yes, rewrite may be painful, but if you can no longer find the people to support the old thing it may be necessary

>the reality is that if there is some code in production that has been working unchanged for the last two years, you're wasting your time refactoring it.

So much this. I recall watching people early in their careers who wanted to make their mark go after code like this that was just a waste of time and more likely than not to blow up things downstream they didn't understand. And sadly watched managers praise their tenacity rather than understanding the explosions that were being created.

I'd say #2 and #3 need to be modified.

#2 Are there related changes on the horizon for the code being refactored?

I think more qualifications than that probably miss times you should refactor.

#3 Has the code been changing recently OR have changes been delayed because modifying the unrefactored code is considered too difficult.

I've seen too many times where unrefactored code is considered too dangerous/difficult to modify even with total test coverage. Refactoring is a necessary step towards self documenting code in those cases.

One reason I might accept one or more NOs to your questions:

Does the refactor support pending work, which isn’t directly related to the refactored code, but benefits from the lessons learned and applied in the refactor… even in some indirect way?

This might be providing a clearer pattern you’ll apply to similar new functionality; or it might be providing a new abstraction or even eliminating a failed abstraction which sets that pending work on the right path.

> Do we have test coverage of the use-cases that are affected?

With statically typed languages and good tooling like JetBrains ReSharper, there are guaranteed safe automated large refactorings that can be done as long as someone isn’t using reflection.m

For tests, when in doubt, I don't touch the original tests and just add my own separately. That way I know that the old tests still validate the originally-intended behavior, and my new tests validate what I expect them to do.

If the tests aren't broken, is there ever a good reason to make sweeping changes to them?

This is a good point! We've gotten some instances of Chesterson's Fence that some devs casually remove during a "test refactor" that later allowed a regression to make it into prod.

I've caught some errors in "test refactoring" from our multiple levels of testing having large overlap with teach other. Our end-to-end tests have a lot of overlap with the integration tests which in turn have a large overlap with unit testing. The unit tests run in a matter of seconds compared to the end-to-end which can take minutes or in some cases hours for our manual testing so the levels of testing also serve as an efficiency for us.

When refactoring tests, some form of mutation coverage [0] would be really nice. Verify that the tests break when the underlying code changes.

[0] https://en.wikipedia.org/wiki/Mutation_testing

Sounds like the tests would benefit from comments explaining what they are testing and why. Just a few days ago there was an article from an experienced dev that after multiple decades he thinks you simply can't put too many comments in tests. I was on the fence with it but seeing your comment, I now know where it comes from.

Maybe this could also be solved with better tooling - tests that allow substituting the tested system with a dummy that fails and thus verify the test is actually working.

I suppose theoretically this must have been behaviour change that crept in with the refactoring, that luckily had a good result and not a bad one.

I get nervous about things that accidentally work but I definitely relate to being so tired or under too much pressure to move on that I just trust in luck.

That said, I would guess that even if the accidental behaviour change didn't occur, the bug would have been much easier to solve after the refactor made the intention of the code clearer.

Same. Sometimes the best way to fix a bug is to refactor it out of existence.

We've got a lot of 20+ year old code in production, a lot written late at night in a crunch. Much of it's ugly, non-optimal and screams to be ripped out and replaced.

But it is working, in areas that see little change and are not overly performance sensitive.

So we only do something with it if we need larger changes. Otherwise we leave it be and spend our time being productive elsewhere.

Great point about team mental context!

Without knowing the context I would guess that this a bad refactor (away from revealing the intention)... probably including some cases of applying new abstractions that don't fit the real problem. How does that match with your experience?

Update: it occurs to me that I have seen complex code (for which people would need a mental map) be simplified so drastically that no-one needs to worry about mental maps. I would much prefer "no-one needs a mental map" over "there's a team with good mental maps".

That is a pain with new guys on team that don’t want to spend time learning the code but rather rewrite it their way so they can understand it.

Only problem is that’s ultimately not polite to existing team and hopefully we can filter such people in hiring process but not always.

The point is not to improve it until you actually need to change it.

Like you said, you're probably bad at predicting which code is going to need changes in the future, so if it's working now and you don't need to change it then it's a silly risk to try to improve it right now.

> Ah, but - in large and poorly-maintained codebase, it is always the case that some refactoring is already in progress, likely stalled for weeks, months or years as priorities have shifted without it having been completed.

This is true, and something I also thought of when reading that point. I don't think it's necessarily a counterargument, though. It's probably a better idea to spend your time helping to complete the previous refactor instead of starting your new one. Codebases in which many refactorings are started but not completed can be worse than ones that aren't refactored at all.

There could be exceptions if your new changes is very small, localized, and unlikely to interfere with the other changes going on.

Original author here :)

> Is anything ever really just a refactor, though?

I think you're making the point that very few worthwhile business goals could be achieved purely by refactoring. Which is true.

Some part of the work will probably be the safe mechanical steps that make up refactoring ("replace conditional with method", "extract method" etc), and some part is "normal" "non-refactoring" change.

The reason for this point is that I see developers avoid important conversations because they claim they're "just refactoring" and so they can safely make a change without involving anyone else. They're wrong unless they have a good handle on the difference between the two types of work.

> it is always the case that some refactoring is already in progress

When I've seen the "paused refactor" phenomenon, it isn't actually refactoring ... there's negotiation and consideration of trade-offs and impacts on various parties, because some important parts of the code need to actually do something different. But probably there are more cases in the world than I've personally seen.

But really in this case I am thinking on the level of developers who will extract a method while at the same time changing the content of the code being replaced. Without running any tests, committing etc. The start and end of the one step is easily in their grasp, but they think they can save time by mixing two steps at once.

> If the code is unlikely to change, there's a good chance you might want to refactor it out of your repository

Interesting point. The cases that were in my head were more like unique things that wouldn't group together well with anything else. What kind of code do you typically move out of a repo?

Would it be fair to say that in the case of extracting unchanging code, we're recognising that there is some difference between our intentions for the code that stays and the code that doesn't? And that by extracting it we're acting on a recognition that the intention has changed since we first wrote it?

> other code _uses_ the code which never changes... And if originally the use is clunky, but you manage to improve it by your refactor

Wouldn't this be more like changing my [previously] unchanging code to make it better fit the use cases of the client?

> Waiting until someone (maybe even yourtself) writes tests for most code

I wouldn't recommend writing tests for most of the code before any refactoring. There is certainly some small part that can be tested and then refactored while the rest stays untested and unchanged. Then on to the next test and refactor and so on.

> who's to say the tests don't fail _already_?

I guess I didn't mention this case explicitly, but I'd expect someone to fix the tests (or scrap the old and write _some_ new) and then get on with refactoring.

Article author here, this is a really good point and I can see how that exchange might undermine the point (or even the entire article) for some readers.

I think I understand it as fighting fire with fire: that people who are enthusiastic about refactoring without understanding and writing tests are prone to pushing their agenda without really engaging on the substantive points (as in someone who talks "refactoring" without taking in how central automated tests are in the refactoring community). Such a person sometimes needs a confrontational push to go back to the basics on this point.