0

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.

6
  • 4
    This is essentially opinion-based. Your team needs to make a decision, and the refactoring should follow the agreed-upon rules. That's not so much about right or wrong but about having common understanding and causing least astonishment when working with code owned by the team, like most coding rules. Some may be universally agreed on, but many are simply matters of convention. Commented Jul 27, 2022 at 10:04
  • 1
    What's "changes in architecture" and what's "make the code testable"? There's often a wide overlap
    – Caleth
    Commented Jul 27, 2022 at 10:38
  • 1
    Does this answer your question? What should take precedence: YAGNI or Good Design?
    – gnat
    Commented Jul 27, 2022 at 11:47
  • The labels used in this question are so vague that it's hard to understand the core problem. Secondly, you're not arguing opposite points. Your coworker may consider it preferable to consider the usage of a pattern beforehand (i.e. during technical analysis), but that says nothing about whether or not you may need to refactor something when it was not yet identified during the technical analysis stage. There are so many possibilities here. Are you overengineering your refactoring? Did you fail to do a proper technical analysis beforehand? Did the requirements change or were they misunderstood?
    – Flater
    Commented Jul 27, 2022 at 12:09
  • [..] Did the pattern only become obvious in hindsight after implementing part of the logic? Because of all of these possibilities, it is not possible to draw a single line in the sand here, making the question hard (if not impossible) to meaningfully answer.
    – Flater
    Commented Jul 27, 2022 at 12:10

2 Answers 2

6

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.

3

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 isn't refactoring as advocated under TDD. In a Red Green Refactor cycle you write the test first. There is no, "refactoring for testability". You're supposed have code that was born testable.

So I'll assume you mean refactoring to retrofit tests as advocated by Michael Feathers in Working Effectively with Legacy Code1, 2. Generally legacy code means proven, working code that has no automated tests. Faced with this we minimize the changes needed to make this existing code testable so we can get its behavior under test without changing that behavior in the process.

Feathers advocates looking for "natural seams" in the code. Like things you can fiddle with in a make file. Start with these and get that behavior locked down. The less code it takes to do this the better because until you do you have no tests. So everything you touch has to be closely reviewed to ensure you didn't change behavior when you made it testable.

Once you do that though it's under test. And you can start refactoring for more testability under the existing test. If that seems redundant that's only because it is.

Tests come in different styles. High level BDD style tests stand way back from your class boundary. That style is what you end up creating first when you go after Feathers seams. There are other tests that insist on speed and isolation. These are very nice. But when retrofitting tests into untested code they shouldn't be first. Retrofitting these tests often requires significant redesign. Not something to do with working code blindly. Get it under a BDD test first.

The aim here is to move where you put your confidence; from the old proven working code to the high level tests. It's much less about proving that the test is correct then about proving that the tests make it do what it did before. The fancy term for that is a regression test. Once you have that in place you are more free to fiddle with the implementation that is making it pass. That fiddling can be changes that let you add more tests.

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

It's good to have a plan if for no better reason then it checks that people know what you mean. When every term we use is overloaded to where it means different things to different people (like the term unit test) it's good to have some way to make what you're expecting people to do clear.

It's also good to remember the old military saying that no plan survives contact with the enemy. Never plan so much that you're not willing to rework the plan.

Plan when it's helping you find problems. The earlier in development process that you find a problem the cheaper it is to fix. But as soon as planning becomes a predictable boring chore that's not helping your team learn anything, move on. It's like popping popcorn. Don't let too much time go between pops.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.