I enjoy refactoring code. There’s a sort of meditative quality in it for me – I get to make something better, one tweak at a time. I also think that refactoring is something you get better at by doing, so when my friend sent me some code over with the challenge “rewrite this without increasing the number of conditionals”, well, I couldn’t resist. This post is a step by step account of my refactoring. The code:

public class BPMClassifier
{
    public static string Classify(int bpm)
    {
        if (bpm > HEART_RATE_ZONES.FAT_BURN.MIN)
        {
            if (bpm > HEART_RATE_ZONES.ENDURANCE.MIN)
            {
                if (bpm > HEART_RATE_ZONES.CARDIO.MIN)
                {
                    if (bpm > HEART_RATE_ZONES.THRESHOLD.MIN)
                    {
                        if (bpm > HEART_RATE_ZONES.PEAK.MIN)
                        {
                            if (bpm > HEART_RATE_ZONES.PEAK.MAX)
                            {
                                return "none";
                            }
                            return "peak";
                        }
                        return "threshold";
                    }
                    return "cardio";
                }
                return "endurance";
            }
            return "fat-burn";
        }

        return "none";
    }
}

public class HEART_RATE_ZONES
{
    public static Zone FAT_BURN = new Zone { MIN = 1 };
    public static Zone ENDURANCE = new Zone { MIN = 5 };
    public static Zone CARDIO = new Zone { MIN = 10 };
    public static Zone THRESHOLD = new Zone { MIN = 15 };
    public static Zone PEAK = new Zone { MIN = 20, MAX = 25 };
}

public class Zone
{
    public int MIN { get; set; }
    public int MAX { get; set; }
}

The Reason to Refactor

Why should we refactor this code? If it’s working fine and doesn’t change often, we shouldn’t refactor it. There is no reason to sit and polish something if you are not going to get a return on your investment.

On the other hand, you should refactor this code if you have to touch it even semi-frequently. My main contention with this code is that it’s complicated. If I were to ask “when does this function return ‘threshold’?”, you’d need a minute to answer. If I want to change the reasons for returning “peak”, I first need to figure out under what conditions we actually return “peak”. Everytime I have to do anything with this function, I need to pay a cognitive tax in order to remember what it actually does.

One last tidbit - did you know that this code suffers from something called the “Christmas Tree” code smell? This code smell refers to the fact that the nested if structure produces a sideways christmas tree:

Christmas Tree Code Smell

Alternatively, I’ve heard this code smell called the Indent Hadouken.

Step 1 - Let There be Tests

When refactoring, our intent is to improve the state of the code without changing any of it’s behavior. As I make changes, I like to have immediate feedback telling me that my changes did not affect the behavior of the code. The best way I know how to do that is to write tests. If I surround the function I intend to refactor with tests, I can run those tests each time I make a change in order to confirm that I have preserved the function’s behavior.

After studying the code for a minute, I come up with my first test case. If the bpm is less than HEART_RATE_ZONES.FAT_BURN.MIN, we return “none”:

  [Test]
  public void should_return_none_if_bpm_is_NOT_greater_than_fat_burn_min()
  {
      var actual = subject(0);

      Assert.AreEqual("none", actual);
  }

The test passes, we’re off to a good start. I then determine that we should return “fat-burn” if bpm is greater than HEART_RATE_ZONES.FAT_BURN.MIN and bpm is less than HEART_RATE_ZONES.ENDURANCE.MIN:

  [Test]
  public void should_return_fat_burn_if_bpm_is_greater_than_fat_burn_min_and_is_NOT_greater_than_endurance_min()
  {
      var actual = subject(5);

      Assert.AreEqual("fat-burn", actual);
  }

Continuing on in this manner, I end up with the rest of the tests:

should_return_endurance_if_bpm_is_greater_than_fat_burn_min_and_is_greater_than_endurance_min_and_is_NOT_greater_than_cardio_min should_return_cardio_if_bpm_is_greater_than_fat_burn_min_and_is_greater_than_endurance_min_and_is_greater_than_cardio_min_and_is_NOT_greater_than_threshold_min should_return_threshold_if_bpm_is_greater_than_fat_burn_min_and_is_greater_than_endurance_min_and_is_greater_than_cardio_min_and_is_greater_than_threshold_min_and_is_NOT_greater_than_peak_min should_return_peak_if_bpm_is_greater_than_fat_burn_min_and_is_greater_than_endurance_min_and_is_greater_than_cardio_min_and_is_greater_than_threshold_min_and_is_greater_than_peak_min_and_is_NOT_greater_than_peak_max should_return_none_if_bpm_is_greater_than_fat_burn_min_and_is_greater_than_endurance_min_and_is_greater_than_cardio_min_and_is_greater_than_threshold_min_and_is_greater_than_peak_min_and_is_greater_than_peak_max

It is worth noting that the structure of our test names looks similar to the structure of our code. Our test names feel complicated because our solution feels complicated.

With our tests in place we can now begin changing some code.

Step 2 - Put Up Your Guards

The first thing I notice is that I can remove the inner-most if statement by adding one more conditional to the outer-most if statement:

  public class BPMClassifier
  {
      public static string Classify(int bpm)
      {
          if (bpm > HEART_RATE_ZONES.FAT_BURN.MIN && !(bpm > HEART_RATE_ZONES.PEAK.MAX))
          {
              if (bpm > HEART_RATE_ZONES.ENDURANCE.MIN)
              {
                  if (bpm > HEART_RATE_ZONES.CARDIO.MIN)
                  {
                      if (bpm > HEART_RATE_ZONES.THRESHOLD.MIN)
                      {
                          if (bpm > HEART_RATE_ZONES.PEAK.MIN)
                          {
                              return "peak";
                          }
                          return "threshold";
                      }
                      return "cardio";
                  }
                  return "endurance";
              }
              return "fat-burn";
          }

          return "none";
      }
  }

The first if statement is now a sort of guard, it only lets in bpm values that are within our range. If the given bpm is outside of our range, we return “none” immediately. This kind of statement is known as a guard clause.

We’ve also removed a duplicate statement – before we had two return "none" statements, we now have one.

Step 3 - Negate All The Things

I think we can simplify this code by using negation. In order to illustrate that, let’s take a quick detour with a small example. Consider the following code:

if (bpm > 1)
{
    if (bpm > 2)
    {
        if (bpm > 3)
        {
            return "greater than 3";
        }

        return "greater than 2";
    }

    return "greater than 1";
}

return "not greater than 1";

When dealing with nested if statements like this, you can negate your conditionals in order to flatten out your code. Let’s apply negation to our first if statement:

if (!(bpm > 1))
{
    return "not greater than 1";
}

if (bpm > 2)
{
    if (bpm > 3)
    {
        return "greater than 3";
    }

    return "greater than 2";
}

return "greater than 1";

By negating the first if statement, we’ve removed one level of indentation. Following this pattern, we can negate the rest of the if statements in our example:

if (!(bpm > 1))
{
    return "not greater than 1";
}

if (!(bpm > 2))
{
    return "greater than 1";
}

if (!(bpm > 3))
{
    return "greater than 2";
}

return "greater than 3";

This code is doing the same thing as the original example, but is much easier to read.

Can we apply this same negation strategy to the code we are refactoring? Let’s try it with the bpm > HEART_RATE_ZONES.ENDURANCE.MIN condtion:

public class BPMClassifier
{
    public static string Classify(int bpm)
    {
        if (bpm > HEART_RATE_ZONES.FAT_BURN.MIN && !(bpm > HEART_RATE_ZONES.PEAK.MAX))
        {
            if (!(bpm > HEART_RATE_ZONES.ENDURANCE.MIN))
            {
                return "fat-burn";
            }

            if (bpm > HEART_RATE_ZONES.CARDIO.MIN)
            {
                if (bpm > HEART_RATE_ZONES.THRESHOLD.MIN)
                {
                    if (bpm > HEART_RATE_ZONES.PEAK.MIN)
                    {
                        return "peak";
                    }
                    return "threshold";
                }
                return "cardio";
            }
            return "endurance";
        }

        return "none";
    }
}

By negating the bpm > HEART_RATE_ZONES.ENDURANCE.MIN condition, we are able to flatten our code a bit. What if we try it to the rest of the if statements:

public class BPMClassifier
{
    public static string Classify(int bpm)
    {
        if (bpm > HEART_RATE_ZONES.FAT_BURN.MIN && !(bpm > HEART_RATE_ZONES.PEAK.MAX))
        {
            if (!(bpm > HEART_RATE_ZONES.ENDURANCE.MIN))
            {
                return "fat-burn";
            }

            if (!(bpm > HEART_RATE_ZONES.CARDIO.MIN))
            {
                return "endurance";
            }

            if (!(bpm > HEART_RATE_ZONES.THRESHOLD.MIN))
            {
                return "cardio";
            }

            if (!(bpm > HEART_RATE_ZONES.PEAK.MIN))
            {
                return "threshold";
            }

            return "peak";
        }

        return "none";
    }
}

Ah, I really like this. The code has the same behavior (we know this because all of our tests still pass), but is much easier to read and just “feels simpler”. The “feels simpler” bit is incredibly important – code that is hard to read or conceptualize slows you down.

Step 4 - Symmetry

The next thing that jumps out at me is the first conditional, if (bpm > HEART_RATE_ZONES.FAT_BURN.MIN && !(bpm > HEART_RATE_ZONES.PEAK.MAX)). It doesn’t feel very symmetrical – it feels like an “outer” if statement for the rest of our code. We can flatten this out if we negate the first part of this condition and use an OR operator:

public class BPMClassifier
{
    public static string Classify(int bpm)
    {
        if (!(bpm > HEART_RATE_ZONES.FAT_BURN.MIN) || bpm > HEART_RATE_ZONES.PEAK.MAX)
        {
            return "none";
        }

        if (!(bpm > HEART_RATE_ZONES.ENDURANCE.MIN))
        {
            return "fat-burn";
        }

        if (!(bpm > HEART_RATE_ZONES.CARDIO.MIN))
        {
            return "endurance";
        }

        if (!(bpm > HEART_RATE_ZONES.THRESHOLD.MIN))
        {
            return "cardio";
        }

        if (!(bpm > HEART_RATE_ZONES.PEAK.MIN))
        {
            return "threshold";
        }

        return "peak";
    }
}

Step 5 - Expansion and Contraction

Sometimes when I refactor code, I go through a process of “expansion and contraction”. In this case we experienced the expansion by negating all of our code. We added additional code in order to flatten out our structure. Now that our code is more readable, let’s see if we can remove the negations that we added.

We can remove the negation from our first if statement if we also flip the > operator in the first part of the condition:

    public class BPMClassifier
    {
        public static string Classify(int bpm)
        {
            if (bpm < HEART_RATE_ZONES.FAT_BURN.MIN || bpm > HEART_RATE_ZONES.PEAK.MAX)
            {
                return "none";
            }

            if (!(bpm > HEART_RATE_ZONES.ENDURANCE.MIN))
            {
                return "fat-burn";
            }

            if (!(bpm > HEART_RATE_ZONES.CARDIO.MIN))
            {
                return "endurance";
            }

            if (!(bpm > HEART_RATE_ZONES.THRESHOLD.MIN))
            {
                return "cardio";
            }

            if (!(bpm > HEART_RATE_ZONES.PEAK.MIN))
            {
                return "threshold";
            }

            return "peak";
        }
    }

We can apply the same contraction to the rest of our if statements by removing the ! and flipping the > operator:

    public class BPMClassifier
    {
        public static string Classify(int bpm)
        {
            if (bpm < HEART_RATE_ZONES.FAT_BURN.MIN || bpm > HEART_RATE_ZONES.PEAK.MAX)
            {
                return "none";
            }

            if (bpm < HEART_RATE_ZONES.ENDURANCE.MIN)
            {
                return "fat-burn";
            }

            if (bpm < HEART_RATE_ZONES.CARDIO.MIN)
            {
                return "endurance";
            }

            if (bpm < HEART_RATE_ZONES.THRESHOLD.MIN)
            {
                return "cardio";
            }

            if (bpm < HEART_RATE_ZONES.PEAK.MIN)
            {
                return "threshold";
            }

            return "peak";
        }
    }

Step 6 - Maps Can Guide Us

Now that we are just looking at a series of if statements, I’m beginning to wonder if we can just use a map. Something like this:

    public class BPMClassifier
    {
        public static string Classify(int bpm)
        {
            var map = new Dictionary<int, string>
            {
                { HEART_RATE_ZONES.PEAK.MIN, "peak"},
                { HEART_RATE_ZONES.THRESHOLD.MIN, "threshold"},
                { HEART_RATE_ZONES.CARDIO.MIN, "cardio"},
                { HEART_RATE_ZONES.ENDURANCE.MIN, "endurance"},
                { HEART_RATE_ZONES.FAT_BURN.MIN, "fat-burn"},
            };


            if (bpm < HEART_RATE_ZONES.FAT_BURN.MIN || bpm > HEART_RATE_ZONES.PEAK.MAX)
            {
                return "none";
            }

            foreach (var zone in map.OrderByDescending(x => x.Key))
            {
                if (bpm > zone.Key) return zone.Value;
            }

            return "none";
        }
    }

I like this new structure, plus all the tests still pass! However, I don’t like that we have two return "none" statements. I think we can solve that with one more condition in the if statement within our foreach loop:

public class BPMClassifier
{
    public static string Classify(int bpm)
    {
        var map = new Dictionary<int, string>
        {
            { HEART_RATE_ZONES.PEAK.MIN, "peak"},
            { HEART_RATE_ZONES.THRESHOLD.MIN, "threshold"},
            { HEART_RATE_ZONES.CARDIO.MIN, "cardio"},
            { HEART_RATE_ZONES.ENDURANCE.MIN, "endurance"},
            { HEART_RATE_ZONES.FAT_BURN.MIN, "fat-burn"},
        };

        foreach (var zone in map.OrderByDescending(x => x.Key))
        {
            if (bpm > zone.Key && bpm <= HEART_RATE_ZONES.PEAK.MAX) return zone.Value;
        }

        return "none";
    }
}

I feel good about this solution, it is less complex and more maleable.

Step 7 - Refactor the Tests

I now want to revisit the tests, our tests names are driving me nuts. Let’s start with the boundaries:

[Test]
public void a_bpm_below_the_fat_burn_minimum_should_map_to_none()
{
    var actual = subject(0);

    Assert.AreEqual("none", actual);
}

[Test]
public void a_bpm_above_the_peak_maximum_should_map_to_none()
{
    var actual = subject(26);

    Assert.AreEqual("none", actual);
}

With these two tests in place, we can now remove the following tests: should_return_none_if_bpm_NOT_greater_than_fat_burn_min

and

should_return_none_if_bpm_is_greater_than_fat_burn_min_and_is_greater_than_endurance_min_and_is_greater_than_cardio_min_and_is_greater_than_threshold_min_and_is_greater_than_peak_min_and_is_greater_than_peak_max

For the rest of our tests, a rename seems appropriate. The next test, should_return_fat_burn_if_bpm_is_greater_than_fat_burn_min_and_is_NOT_greater_than_endurance_min, can be renamed to should_return_fat_burn_if_bpm_is_within_fat_burn_range. Following a similar pattern with the rest of our tests, we end up with the following test suite:

public class tests
{
    Func<int, string> subject = BPMClassifier.Classify;

    [Test]
    public void a_bpm_below_the_fat_burn_minimum_should_map_to_none()
    {
        var actual = subject(0);

        Assert.AreEqual("none", actual);
    }

    [Test]
    public void a_bpm_above_the_peak_maximum_should_map_to_none()
    {
        var actual = subject(26);

        Assert.AreEqual("none", actual);
    }

    [Test]
    public void should_return_fat_burn_if_bpm_is_within_fat_burn_range()
    {
        var actual = subject(4);

        Assert.AreEqual("fat-burn", actual);
    }

    [Test]
    public void should_return_endurance_if_bpm_is_within_endurance_range()
    {
        var actual = subject(9);

        Assert.AreEqual("endurance", actual);
    }

    [Test]
    public void should_return_cardio_if_bpm_is_within_cardio_range()
    {
        var actual = subject(14);

        Assert.AreEqual("cardio", actual);
    }

    [Test]
    public void should_return_threshold_if_bpm_is_within_threshold_range()
    {
        var actual = subject(19);

        Assert.AreEqual("threshold", actual);
    }

    [Test]
    public void should_return_peak_if_bpm_is_within_peak_range()
    {
        var actual = subject(24);

        Assert.AreEqual("peak", actual);
    }
}

Some Final Tweaks

We are in a great spot. I have a few more minor refactors that I’d like to do. Since we are in C#, I’d like to move the map outside of our function:

public class BPMClassifier
{
    static Dictionary<int, string> map = new Dictionary<int, string>
    {
        { HEART_RATE_ZONES.PEAK.MIN, "peak"},
        { HEART_RATE_ZONES.THRESHOLD.MIN, "threshold"},
        { HEART_RATE_ZONES.CARDIO.MIN, "cardio"},
        { HEART_RATE_ZONES.ENDURANCE.MIN, "endurance"},
        { HEART_RATE_ZONES.FAT_BURN.MIN, "fat-burn"},
    };

    public static string Classify(int bpm)
    {
        foreach (var zone in map.OrderByDescending(x => x.Key))
        {
            if (bpm > zone.Key && bpm <= HEART_RATE_ZONES.PEAK.MAX) return zone.Value;
        }

        return "none";
    }
}

I don’t like that we return “none” only if we don’t find a match within our map. We can fix that with a guard clause:

public static string Classify(int bpm)
{
    if (bpm < HEART_RATE_ZONES.FAT_BURN.MIN || bpm > HEART_RATE_ZONES.PEAK.MAX)
    {
        return "none";
    }
    else
    {
        foreach (var zone in map.OrderByDescending(x => x.Key))
        {
            if (bpm > zone.Key) return zone.Value;
        }
    }

    return "none";
}

We are now back to our previous problem of returning “none” twice. We can address this by using a C# Linq function:

public static string Classify(int bpm)
{
    if (bpm < HEART_RATE_ZONES.FAT_BURN.MIN || bpm > HEART_RATE_ZONES.PEAK.MAX)
    {
        return "none";
    }

    return map.First(zone => bpm > zone.Key).Value;
}

And with that, we have our final state:

public class BPMClassifier
{
    static Dictionary<int, string> map = new Dictionary<int, string>
    {
        { HEART_RATE_ZONES.PEAK.MIN, "peak"},
        { HEART_RATE_ZONES.THRESHOLD.MIN, "threshold"},
        { HEART_RATE_ZONES.CARDIO.MIN, "cardio"},
        { HEART_RATE_ZONES.ENDURANCE.MIN, "endurance"},
        { HEART_RATE_ZONES.FAT_BURN.MIN, "fat-burn"},
    };

    public static string Classify(int bpm)
    {
        if (bpm < HEART_RATE_ZONES.FAT_BURN.MIN || bpm > HEART_RATE_ZONES.PEAK.MAX)
        {
            return "none";
        }

        return map.First(zone => bpm > zone.Key).Value;
    }
}

public class HEART_RATE_ZONES
{
    public static Zone FAT_BURN = new Zone { MIN = 1 };
    public static Zone ENDURANCE = new Zone { MIN = 5 };
    public static Zone CARDIO = new Zone { MIN = 10 };
    public static Zone THRESHOLD = new Zone { MIN = 15 };
    public static Zone PEAK = new Zone { MIN = 20, MAX = 25 };
}

public class Zone
{
    public int MIN { get; set; }
    public int MAX { get; set; }
}

I hope that by showing each stage in our refactoring, you’re now better equipped to deal with nested if statements. I’d love to hear what you think.