Unit Tests Anti-patterns: TDD without refactoring

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.