Unit Tests Anti-patterns: TDD without refactoring

Standard
This series goes through anti-patterns when writing tests. Yes, there are and will be many. 
TDD without refactoringLogic in testsMisleading testsNot asserting
Code matchingData transformationAsserting on not nullPrefixing test names with "test"

Anti-patterns are patterns with negative consequences. In this series I’m going to tackle those patterns, explain what they signify in terms of risks, and what to do instead.

Let’s look at a pattern common to people who start out on their TDD journey: They forget to refactor.

When they do, it starts out like this. Here’s the first test of a lightsaber’s color.

and the code:

Then add a second test:

and code:

And a third test:

and code:

Ok, one more:

And the code keeps growing like this (just the GetColor method):

We can spot a couple of symptoms regarding the no-refactoring pattern. The first is the tested code, that gets longer and longer with every test. The more tests we add, the code under test gets longer and longer (usually in bigger rate than the tests).

If we replace the if-else code with a map (Dictionary in C#), for example, adding support for another color becomes one line.

By refactoring we generalize the code, so adding behaviors becomes simpler. While this anti-pattern is obvious just with conditionals, it gets worse with extensive logic. The methods becomes longer, procedural and become a spaghetti monster. Instead, we can extract classes and methods to simplify.

But that’s not all, folks. Many times, we’ll see the copy-pasting of the tests, as in this case. It’s easy to just copy the whole tests, instead of refactoring. Refactoring is valuable, so let’s do it:

It’s not like we’re going to eliminate the copy-pasting. I use it myself. But the earlier we’ll refactor the tests, we’ll copy and change less code, keep tests lean, maintainable, readable, and eliminate the occasional copied bug.

6 thoughts on “Unit Tests Anti-patterns: TDD without refactoring

  1. If one is not spending a good amount of time on factor, the are “doing it wrong”.

    For highly effective teams, this can be 70% or more of the actual invested time….

    1) RED – write the smallest test that fails because of a missing capability
    2) GREEN – write the smallest amount of code as quickly as possible to pass the test
    3) REFACTOR – now that there is code that is “functional” make it code that is quality.

    For highly effective teams, I have seen rates as high as

    1) TEST Creation : 2 Minutes
    2) CODE Pass : 3 minutes
    3) REFACTOR: 40 minutes

    • Gil Zilberfeld

      Thanks David!

      I think the numbers should be taken in context, though. Test creation, if taken in small steps, will probably take around that time for well designed code. Throw in dependencies, legacy and other situations, test creation (especially the first “meaningful” one, before starting to mutate it) can take a lot longer. Refactoring takes its toll too, but I don’t see this kind of proportions consistently with every test.

      • Gil,

        100% agreement. That is why my post said “as high as”, and not “typical”, “average”, etc.

        When there is existing code, there are indeed challenges. But there are also helpful techniques.

        Remember, a test written by the person writing the code (or even reading it) can not really determine if the code if “right” or “wrong”. They can only determine if the code does “what I expected” or not. [As a trivial example, consider if the specification they are working to is actually incorrect].

        Thus for existing (presumed working) code, there is rarely need create a large suite of “meaningful” tests. Rather a technique known as “pinning tests” can be quite effective. Pinning tests characterize the current behavior and fail in the case of a change in behavior.

        This can save a tremendous amount of time as Pinning Tests can easily be generated. Not only to provide code coverage [each line executed] but also cyclomatic complexity [every possible path executed].

        So many techniques available, and selecting the appropriate set for a given task, in a given environment, with a given team is always a worthy endeavor.

  2. Nimrod

    While I agree that refactoring is a crucial part of keeping your code clean and well designed, there is also the issue of premature refactoring. Eliminating duplication too early, for example, could lead to creating the wrong abstractions, since you’re not really understanding the domain yet.

    • Gil Zilberfeld

      Thanks Nimrod!

      Yes, and…
      Any code needs to be reviewed, and that means you’ll get two sets of eyes (at least) on the code. Code reviews are the last guardian of any stepping out of standards. So assuming we all make mistakes (sometimes we don’t know we’re making them), there’s a safety net for that.
      You can identify most of the errors through feedback, and premature refactoring can be identified only later in the process. We can’t really know it is premature when we do it. Rely on other practices, like code reviews to mitigate.

    • Nimrod – While I agree that it may be inappropriate to perform a certain refactoring while there are still major gaps in knowledge, there are also many cases (much more IMPO) where immediate (and often repeated) refactoring is key.

      This is there the “continual improvement” comes in with adopting any practice. At the beginning I would much rather see a bit of inefficient refactoring than to see refactoring opportunities missed.

Leave a Reply

Your email address will not be published. Required fields are marked *