When not to refactor

When not to refactor
Posted by Lars Vonk around lunchtime: July 18th, 2008

Refactoring is part of a software developers work. General rule of thumb I always used is if you see code that smells (for instance duplication) you should refactor. But does this rule always apply? Are there any situations when you should not refactor? And if so, how do you recognize those situations?

Last week I worked on a particular task, let's call it task X, where I needed to use a service with an unclear interface and a smelly implementation. After consulting with the rest of the team we decided to refactor this particular method of the service so it will be more clear and remove code duplication in that method. But while I was refactoring I noticed the other methods in the service were also unclear and contained code duplication, so I started to refactor those as well. To proof the service still worked as before I needed a good (automated) regression testset. It was production software and I wanted to make sure I didn't break anything with my refactoring. Unfortunately there wasn't an automated regression testset for all the methods in the service. So while I was on the verge of creating a complete automated regression testset for the entire service that, amongst other things, would have consisted of mocking a webservice call I remembered something about shaving yaks.
Why would I create an automated testset for the entire service, while I am actually working on task X? So before stuff get out of hand I decided not to refactor the other methods in the service, leave the code duplication in place and just stick to refactoring (and testing) that one method in the service.

What made me decide not to refactor?

The refacoring did not have a direct connection with task X I was working on
There was no automated regression testset available
The rest of team agreed with not refactoring
The complete refactoring (incl testing) would have taken more than a single day to finish (at least a week)
A good practice in agile modeling is only update when it hurts. I think this also applies when it comes to refactoring code. I wasn't using the other methods of the service and therefor it did not hurt me

With pain in my heart I had to leave the smelly code alone and went on with implementing Task X. Does this mean the service stays smelly? I think that will depend on the future functionality we are going to implement and if we need that same service again. And if we do we will probably apply the same rules as we did in this situation.

Lars

Filed under Java | 8 Comments »

8 Responses to “When not to refactor”

Age Mooy Says:

Posted at: July 18, 2008 at 1:13 pm

Did you add the lack of a good regression test to your backlog as a user story or an impediment ? I think you made the correct short-term decision but that does not mean the problem goes away.

If you make these kinds of short-term decisions too often, you end up with (more) technical debt until it reaches a scale where you can’t even fix it within one iteration anymore. Usually somewhere around that point you do have to change the code involved but by that time it is no longer possible to refactor it due to overwhelming technical debt.

Deciding to not refactor can be a good thing but you should always try to make sure that the problems you found does get solved at some point before they become major impediments.

Nanne Says:

Posted at: July 18, 2008 at 1:16 pm

There is a good book ‘Working Effectively with Legacy Code by Michael Feathers’ about this subject. It describes, amongst others, the situation you described above. The book offers pattern/solutions to problems you might encounter while working with legacy code. It addressed some problems I encounter on a daily basis.

Frank Smith Says:

Posted at: July 19, 2008 at 11:10 pm

A good test suite is a prerequisite for effective refactoring. So this is one answer to your questions. If the team doesn’t have good testing practices and it’s not willing or able to pay that debt then refactoring should be done very carefully, if at all.

It’s analogous to asking “when should someone not exercise?”. If the person has a serious heart condition from years of smoking and being obese, they might want to consider exercising less than is normally recommended.

Mathias Says:

Posted at: July 20, 2008 at 12:07 am

I agree with Age Mooy. It sounds like the right decision in the short term: starting to refactor right now, just for one feature, with no tests in place, sounds like entering the jungle with just a pocket knife.
On the other hand, the key issue here is the absence of tests; that’s not going to improve over time, and will make changes more and more painful over time.
I would discuss with the team and see whether there is an agreement to invest some time in putting in place the minimum missing tests so that next time you don’t have the issue!

Gerard Janssen Says:

Posted at: July 20, 2008 at 8:34 am

Age has an interesting take on this question. It sounds reasonable, but I’m not sure if I agree with it. Sure it is useful to add a user story to make the technical debt visible. But I do not agree that the technical debt was actually increased by this action. It was already there, only hidden. So it remains the question what the long term effect will be. If it will never hurt enough, it is best left alone. I agree that when building new software you should optimize your unit testing (although taking that to far might also get you into trouble as we have experienced in the past…), but when working with existing code you do need to be careful not to go to far with this.
Writing unit tests is done for a purpose. In this case for being able to refactor smelly code. But smelly code is not the same as broken code. I think in this case the right decision was made to not invest the clients money to refactor, but to save it for adding value in the form of new features. So how about stating that not having unit tests for smelly code is not technical debt at all and that not having unit test for broken code or for code that needs to be changed consists of technical debt. So is it technical debt? It depends. For new code this is different. Here you create unit tests to verify you new code works, so the decision is much easier made to write unit tests.
Just writing unit tests for every piece of existing code is not a wise investment of money. If you are not going to need it, don’t.

Sara Says:

Posted at: July 21, 2008 at 4:54 am

Very good points. I would also maintain that if the smelly code is driving you crazy enough to refactor in your spare time none of these rules apply.

Manish Sharma Says:

Posted at: August 12, 2008 at 8:08 am

Avoiding refactoring due to lack of time and other factors is a commonly observed in software development. But one thing commonly observed is that we either try to refactor full code or none whereas I believe refactoring is a step by step process.

Taking you case as example, I would have refactored the duplicate code (visible smell) and moved on with Task X. Sometime later when I or someone else visits the service class then refactor a bit more with unit tests.

nothing blog from outer space : To refactor or not to refactor Says:

Posted at: July 28, 2009 at 4:07 pm

[...] Das Refactoring von grösseren Projekten bringt jedoch auch Risiken mit sich: Zum einen ist schwer abzuschätzen, wann der Code für die Veröffentlichung „schön genug“ ist, und Refactoring kann zum teuren Selbstläufer werden. Zum anderen besteht eine reale Gefahr, vorhandenen Code zu beschädigen: Denn die Einzelteile des bestehenden Systems sind unter Umständen stärker miteinander verwoben, als dies auf den ersten Blick ersichtlich ist. Eine unbedachte Änderung einer Methode kann andernorts zu unerwünschten Nebeneffekten führen. Automatisierte Tests helfen, dieses Risiko zu verringern, doch auch diese müssen sorgfältig erstellt werden und die richtigen Bedingungen testen. Schliesslich ist eine sorgfältige Zeitplanung notwendig, um ein Projekt trotz Refactoring fristgerecht abliefern zu können – auch schöner Code nützt wenig, wenn er nie produktiv eingesetzt wird. (Gründe, auf ein Refactoring zu verzichten.) [...]