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.

[TestMethod]
public void Yoda_Lightsaber_Color_Is_Green()
{
    LightSaber lightsaber = new LightSaber();
    lightsaber.BelongsTo("Yoda");
    Assert.AreEqual(LightSaberColor.Green, lightsaber.GetColor());
}

and the code:

public class LightSaber
{
    public void BelongsTo(string name)
    {
        
    }

    public LightSaberColor GetColor()
    {
        return LightSaberColor.Green;
    }
}

Then add a second test:

[TestMethod]
public void Luke_Lightsaber_Color_Is_Blue()
{
    LightSaber lightsaber = new LightSaber();
    lightsaber.BelongsTo("Luke");
    Assert.AreEqual(LightSaberColor.Blue, lightsaber.GetColor());
}

and code:

public class LightSaber
{
    string owner;
    public void BelongsTo(string name)
    {
        owner = name;
    }

    public LightSaberColor GetColor()
    {
        if (owner == "Yoda")
            return LightSaberColor.Green;
        else
            return LightSaberColor.Blue;
    }
}

And a third test:

[TestMethod]
public void Vader_Lightsaber_Color_Is_Red()
{
    LightSaber lightsaber = new LightSaber();
    lightsaber.BelongsTo("Darth Vader");
    Assert.AreEqual(LightSaberColor.Red, lightsaber.GetColor());
}

and code:

public class LightSaber
{
    string owner;
    public void BelongsTo(string name)
    {
        owner = name;
    }

    public LightSaberColor GetColor()
    {
        if (owner == "Yoda")
            return LightSaberColor.Green;
        else
            if (owner == "Luke")
            return LightSaberColor.Blue;
        else
            return LightSaberColor.Red;
    }
}

Ok, one more:

[TestMethod]
public void Windu_Lightsaber_Color_Is_Purple()
{
    LightSaber lightsaber = new LightSaber();
    lightsaber.BelongsTo("Mace Windu");
    Assert.AreEqual(LightSaberColor.Purple, lightsaber.GetColor());
}

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

public LightSaberColor GetColor()
{
    if (owner == "Yoda")
        return LightSaberColor.Green
    else
        if (owner == "Luke")
        return LightSaberColor.Blue;
    else
        if (owner == "Darth Vader")
        return LightSaberColor.Red;
    else
        return
            LightSaberColor.Purple;
}

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.

Dictionary<String, LightSaberColor> saberColorList =
    new Dictionary<string, LightSaberColor>()
    {
        {"Yoda", LightSaberColor.Green },
        {"Luke", LightSaberColor.Blue },
        {"Darth Vader", LightSaberColor.Red },
        {"Mace Windu", LightSaberColor.Purple }
    };

public LightSaberColor GetColor()
{
    return saberColorList[owner];
}

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:

public class LightSaberTests
{
    LightSaber lightsaber;
    [TestInitialize]
    public void Setup()
    {
        lightsaber = new LightSaber();

    }
    [TestMethod]
    public void Yoda_Lightsaber_Color_Is_Green()
    {
        lightsaber.BelongsTo("Yoda");
        Assert.AreEqual(LightSaberColor.Green, lightsaber.GetColor());
    }

    [TestMethod]
    public void Luke_Lightsaber_Color_Is_Blue()
    {
        lightsaber.BelongsTo("Luke");
        Assert.AreEqual(LightSaberColor.Blue, lightsaber.GetColor());
    }

    [TestMethod]
    public void Vader_Lightsaber_Color_Is_Red()
    {
        lightsaber.BelongsTo("Darth Vader");
        Assert.AreEqual(LightSaberColor.Red, lightsaber.GetColor());
    }

    [TestMethod]
    public void Windu_Lightsaber_Color_Is_Purple()
    {
        lightsaber.BelongsTo("Mace Windu");
        Assert.AreEqual(LightSaberColor.Purple, lightsaber.GetColor());
    }
}

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.

Categories: Uncategorized

6 Comments

David V. Corbin · July 21, 2016 at 11:14 pm

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 · July 22, 2016 at 9:44 am

    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.

      David V. Corbin · July 22, 2016 at 12:40 pm

      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.

Nimrod · July 27, 2016 at 9:09 pm

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 · August 1, 2016 at 12:25 pm

    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.

    David V. Corbin · August 1, 2016 at 12:32 pm

    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

Avatar placeholder

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