Title: The Refactor Part of TDD – Reducing to the Min with FizzBuzz

Author: Bob Schmidt

Date: 07 January 2020 18:03:04 +00:00 or Tue, 07 January 2020 18:03:04 +00:00

Summary: Simon Sebright considers whether de-duplication in refactoring can be too aggressive.

Body: 

I recently had a little time on my hands, at the same time a clearish head and a laptop with a development environment on it, so decided to keep my eye in with a short FizzBuzz TDD kata session.

The design part of the exercise was minimal (one function), and although I added a few more tests during the session, my main focus was on the Refactor (of Red, Green, Refactor) part – seeing what happens if I try to ruthlessly reduce things to the minimum. This means removing duplication and potentially superfluous concepts/complications.

The result, as can be seen on GitHub (a .Net Core Assembly project in Visual Studio 2019 [1]), raised a few questions in my mind as to how far one can/should go down that track. See for yourself, what you think about readability, understandability, maintainability, extensibility, etc., of the different versions of the code...

Getting to the FizzBuzz interface

First, I briefly describe how I ended up with the interface to the core logic.

With the first test:

  [TestMethod]
  public void Start()
  {
    Assert.AreEqual(1, FizzBuzzer.Eval(1));
  }

I generated a static method in my FizzBuzz class using IDE refactoring. It was red first (throws exception). Then I made it green:

  static class FizzBuzzer
  {
    internal static object Eval(int v)
    {
      return 1;
    }
  }

Next came more tests (the Red of Red, Green, Refactor) and I went through a couple of versions:

  internal static object Eval(int v)
  {
    return v == 3 ? (object)"Fizz" : v;
  }

  ...

Before arriving at the final interface (returns a string now) and a working implementation:

  internal static string Eval(int v)
  {
    return v%15 == 0 ? "FizzBuzz":
        v%3 == 0 ? "Fizz" :
          v%5 == 0 ? "Buzz" :
            v.ToString();
  }

So, as we can see I take an integer being the number currently being ‘played’ in a game and return as a string the correct answer, which a player should say aloud.

At this point, I could have stopped after making sure I had a suitably full set of tests (see Listing 1 for the tests). Also, I could have considered what happens with 0 or negative input. I decided to leave those (as they actually produce something sensible) and look at the refactoring opportunities.

[TestClass]
public class FizzBuzzTest
{
  [TestMethod]
  public void Numbers()
  {
    Assert.AreEqual("1", FizzBuzzer.Eval(1));
    Assert.AreEqual("2", FizzBuzzer.Eval(2));
    Assert.AreEqual("4", FizzBuzzer.Eval(4));
    Assert.AreEqual("7", FizzBuzzer.Eval(7));
  }
  [TestMethod]
  public void Singles()
  {
    Assert.AreEqual("Fizz", FizzBuzzer.Eval(3));
    Assert.AreEqual("Buzz", FizzBuzzer.Eval(5));
  }
  [TestMethod]
  public void Multiples()
  {
    Assert.AreEqual("Fizz", FizzBuzzer.Eval(6));
    Assert.AreEqual("Buzz", FizzBuzzer.Eval(10));
  }
  [TestMethod]
  public void Combined()
  {
    Assert.AreEqual("FizzBuzz",
      FizzBuzzer.Eval(15));
    Assert.AreEqual("FizzBuzz",
      FizzBuzzer.Eval(30));
  }
}
			

Removing duplication and complications

There are some things being duplicated in the code:

• The use of the ternary operator, three times
• 3 and 5 are duplicated in 15 (as prime factors)
• The string literals "Fizz" and "Buzz" are to be found in the compound "FizzBuzz"

The first thing I did was to address the 15 and the "FizzBuzz". I could do this in one go by concatenating the results of the two factors (see Listing 2).

internal static string Eval(int v)
{
  string fizzness = v % 3 == 0 ? "Fizz" :
    String.Empty;
  string buzzness = v % 5 == 0 ? "Buzz" :
    String.Empty;
  string fizzBuzzness = fizzness + buzzness;
  return String.IsNullOrEmpty(fizzBuzzness) ?
    v.ToString() : fizzBuzzness;
}
			

It is perhaps clearer that the result is compounded from the two factors, the name fizzBuzzness intending to convey this concatenation (as opposed to declaring a result string and adding both to it in turn).

Note that we still have duplication of the ternary operator. We can get rid of that by implementing a local function to take care of that logic (see Listing 3).

internal static string Eval(int v)
{
  string Test(int test, int divisor,
    string whatIfDivisible)
  {
    return test % divisor == 0 ? whatIfDivisible :
      String.Empty;
  }
  string fizzness = Test(n, 3, "Fizz");
  string buzzness = Test(n, 5, "Buzz");
  string fizzBuzzness = fizzness + buzzness;
  return String.IsNullOrEmpty(fizzBuzzness) ?
    n.ToString() : fizzBuzzness;
}
			

But, we now have duplication of calling the Test() function twice. We can remove that using a loop (Listing 4).

internal static string Eval(int n)
{
  string Test(int test, int divisor,
    string whatIfDivisible)
  {
    return test % divisor == 0 ? whatIfDivisible :
      String.Empty;
  }

  var factors = new[]
  { new { divisor = 3, term = "Fizz" },
    new { divisor = 5, term = "Buzz" } };
  string factorterms = string.Empty;

  foreach (var factor in factors)
  {
    factorterms += Test(n, factor.divisor,
      factor.term);
  }

  return String.IsNullOrEmpty(factorterms) ?
    n.ToString() : factorterms;
}
			

This also has the effect of putting the ‘data’ for the program in one place. So rather than having hard-coded values in multiple lines, it’s all in one bundle. I chose to use an anonymous class to hold the divisor data, to save introducing a new named class which would only be used in this context.

Notice that the Eval() function is now getting longer in terms of lines of code, although we have removed some duplication. That is not a bad thing.

We can now remove the loop by observing that there is a built-in function which will go through a collection in order and perform an action on successive elements, accumulating a result. For example, with a sequence of integers, a ‘seed’ value of 0, a function adding each successive element to the current sum will give you the sum of all the elements. Or a seed of 1 can be used when multiplying to give the result of multiplying all the numbers in the collection together (lowest common denominator if your numbers are prime). Taking a starting seed value of an empty string, we can simply concatenate the results of each factor (see Listing 5).

internal static string Eval(int n)
{
  string Test(int test, int divisor,
    string whatIfDivisible)
  {
    return test % divisor == 0 ? whatIfDivisible :
      String.Empty;
  }

  var factors = new[]
  { new { divisor = 3, term = "Fizz" },
    new { divisor = 5, term = "Buzz" } };
  string factorterms =
    factors.Aggregate(String.Empty, (a, b) =>
    { return a + Test(n, b.divisor, b.term); });
  return String.IsNullOrEmpty(factorterms) ?
    n.ToString() : factorterms;
}
			

The Aggregate() function is our friend here in .Net. Ruby has accumulate() and I am sure other languages have their equivalent.

The next step could be to take the contents of the Test() function and pass this directly as a lambda to Aggregate() (Listing 6).

internal static string Eval(int n)
{
  var factors = new[]
  { new { divisor = 3, term = "Fizz" },
    new { divisor = 5, term = "Buzz" } };
  string factorterms =
    factors.Aggregate(String.Empty, (a, b) => {
      return a + (n % b.divisor == 0 ? b.term :
      string.Empty); });
  return String.IsNullOrEmpty(factorterms) ?
    n.ToString() : factorterms;
}
			

I am still slightly annoyed that in the last line we have to reference factorTerms twice. In a language where an empty string evaluates to the boolean value false, we might be able to get around that. Another go might be to find or write a function which is specifically designed to take the first non-empty string argument from a list. This makes the Eval() function slightly simpler, but we would need tests for the new function. Listing 9 (below) gives an example. I chose to pass objects to it, thus delaying the need to turn the integer n into a string in the Eval() function. Listing 7 is the result of that process.

internal static string Eval(int n)
{
  var factors = new[]
  { new { divisor = 3, term = "Fizz" },
    new { divisor = 5, term = "Buzz" } };
  string factorterms =
    factors.Aggregate(String.Empty, (a, b) => {
      return a + (n % b.divisor == 0 ? b.term :
      string.Empty); });
  return FirstNonEmpty(new object[] {
    factorterms, n });
}
			

Hopefully, their names indicate what the function is doing, although I rather think Aggregate() is too generic – you need to look at the seed and function provided to work out what it is actually going to do. Knowing that .Net has the String.Concat() function, I broke the Aggregate() into two steps. Hopefully the naming of the intermediate variable makes it clear what the Select() is doing (picking out the strings of the divisible factors). Listing 8 could be the result.

internal static string Eval(int n)
{
  var factors = new[]
  { new { divisor = 3, term = "Fizz" },
    new { divisor = 5, term = "Buzz" } };
  IEnumerable<string> relevantFactorTerms =
    factors.Select((factor) => {
      return (n % factor.divisor ==
      0 ? factor.term : string.Empty); });
  string factorterms =
    String.Concat(relevantFactorTerms);
  return FirstNonEmpty(new object[] {
    factorterms, n });
}
			

Is smaller better, what is smaller?

So now we have a function of ‘only’ 4 lines (depending on your preferred line length, we could also inline the variable factorTerms), and practically no logic in the implementation. The only real logic is the divisibility test, which is, after all, the core of FizzBuzz. Instead, we rely on other functions to do the hard work for us, combining them in a crafty way.

internal static string FirstNonEmpty(object[]
  values)
{
  foreach (object o in values)
  {
    string s = o.ToString();
    if (!String.IsNullOrEmpty(s))
    {
      return s;
    }
  }

  throw new ArgumentException("FirstNonEmpty
  given collection with no empty strings");
}
			

Now, the interesting questions come. Imagine you are involved in this codebase. You are in a team with people both less and more experienced than you. Your team is responsible for this code, and possible changes to the game, or new but similar games.

Looking back through the various incantations of Eval(), which do you feel more comfortable with, given the following scenarios:

• It is just you maintaining this code
• You have juniors on your team
• You are all senior on the team
• Your product owner decides to change to 5 and 7 as prime factors
• You update the game to include any number which contains the factors as strings (e.g. 13, 23, 31 etc. for 3 and 51, 52, 53, etc.) for 5
• You need to make a new game FizzBuzzBazz with factors 3, 5 and 7

Conclusion

I certainly found the exercise interesting and illuminating, seeing how naive or ‘clever’ one can be with the same piece of functionality. In a normal day job, such questions will be constantly arising when working on code. Now and then it pays to think about your target audience of (you and) your co-developers.

Reference

[1] FizzBuzz: https://github.com/SimonSebright/FizzBuzz

Simon Sebright Simon has been in Software and Solution development for over 20 years, with a focus on code quality and good practice.

Notes: 

More fields may be available via dynamicdata ..