Software Engineering
refactoring
Updated Mon, 19 Sep 2022 18:41:58 GMT

When is it practical to design in advance of refactoring?


I have a colleague that insists refactoring to make the code testable and introduce tests should be independent from changes in architecture as part of refactoring, e.g. introducing a factory pattern. They think that such design decisions, like introducing a factory pattern, should be conceived and discussed with the team up front, before we start changing any code. I believe design changes are inherently a part of refactoring, whether it's solely to make the code testable or for any other valid reason to refactor.

Should we limit refactoring to only what's necessary to make it testable if that is our primary goal? Is it appropriate to introduce additional design changes as we refactor for testability? Is it practical to conceive and decide on design changes in advance before refactoring begins?

This is about design changes as a part of refactoring decisions, not design to change the behavior.




Solution

Should we limit refactoring to only what's necessary to make it testable if that is our primary goal?

If that would be your only goal, the answer is clearly yes - why should you invest time into things which you don't aim for?

However, testability is rarely an exclusive goal. When writing potentially long-living code, you and your team will typically have additional goals like readability or extensibility, and refactoring is the way to achieve these goals.

Is it practical to conceive and decide on design changes in advance before refactoring begins?

Where I work, we usually don't see most refactorings as a step on its own, or justified by themselves. Instead, if we want to fix a bug, add a new feature or optimize resource usage, we check if the involved area in code is

  • readable enough to reason about the bug, its fix, or the planned extension

  • extensible enough to add the new feature and

  • testable enough to make sure the former code changes can be validated by automated test.

If it is not, one has to refactor the code as they go, either before, after or in between the intended changes - these refactorings are inherently part of any coding work, nothing you find as a separately planned step in anyones personal calendar. Hence, it makes rarely sense to make a huge discussion with the team about an isolated refactoring which is clearly sensible to achieve the real primary goals - getting new features into the software, and the bugs out of it. Don't forget, refactoring is not an end in itself, it is a means to an end.

That does not mean you should not discuss larger refactorings with other team members. If you are unsure about the cost/benefit relationship of a refactoring, or in case you are going to change a name in the code base which is used in hundreds of places, or in case you see a risk of breaking things, an up-front discussion makes obviously sense. But it makes zero sense to discuss every small, minor refactoring like changing a local variable's name, or extracting a small function, or adding or improving a comment line with the whole team beforehand. It is totally sufficient to let these changes be checked by the reviewer who looks at your pull request.

Where to draw the line exactly, which issues are "large enough" to be discussed with the team up-front, and which not, that is something you need to work out with your team by yourself.

Let us finally look at your example of "introducing a factory". One has to look for the reasons why a factory is introduced:

  • is it for increasing testability, for example by allowing to produce mock objects, or for making a complex object creation process separately testable?

  • or is it for implementing a specific extension, and making the system more extensible beforehand will lower the effort? For example, by introducing some generic or abstract factory which will allow to introduce new subclasses of a base class with less changes?

  • or do you just think of it as a kind of strategic refactoring, to bring different construction code for certain subclasses into a common place, but with no obvious direct benefit?

Specificially the last reason is not clearly motivated by a direct feature addition or bug fix or obvious improvement, hence a good candidate for a discussion with your colleagues.