Ways to improve my FizzBuzz solution for a TDD role? [closed]

Question

I had an interview recently where I was asked to produce the traditional FizzBuzz solution:

Output a list of numbers from 1 to 100.

• For all multiples of 3 and 5, the number is replaced with "FizzBuzz"
• For all remaining multiples of 3, the number is replaced with "Fizz"
• For all remaining multiples of 5, the number is replaced with "Buzz"

My solution was written in Java because of the role, but this was not a requirement. The interviewer was keen to see some evidence of TDD, so in that spirit I went about producing my very own home-grown FizzBuzz unit test:

public class FizzBuzzTest {

    @Test
    public void testReturnsAnArrayOfOneHundred() {
        String[] result = FizzBuzz.getResultAsArray();
        assertEquals(100, result.length);
    }

    @Test
    public void testPrintsAStringRepresentationOfTheArray() {
        String result = FizzBuzz.getResultAsString();
        assertNotNull(result);
        assertNotSame(0, result.length());
        assertEquals("1, 2", result.substring(0, 4));
    }

    @Test
    public void testMultiplesOfThreeAndFivePrintFizzBuzz() {
        String[] result = FizzBuzz.getResultAsArray();

        // Check all instances of "FizzBuzz" in array
        for (int i = 1; i <= 100; i++) {
            if ((i % 3) == 0 && (i % 5) == 0) {
                assertEquals("FizzBuzz", result[i - 1]);
            }
        }
    }

    @Test
    public void testMultiplesOfThreeOnlyPrintFizz() {
        String[] result = FizzBuzz.getResultAsArray();

        // Check all instances of "Fizz" in array
        for (int i = 1; i <= 100; i++) {
            if ((i % 3) == 0 && !((i % 5) == 0)) {
                assertEquals("Fizz", result[i - 1]);
            }
        }
    }

    @Test
    public void testMultiplesOfFiveOnlyPrintBuzz() {
        String[] result = FizzBuzz.getResultAsArray();

        // Check all instances of "Buzz" in array
        for (int i = 1; i <= 100; i++) {
            if ((i % 5) == 0 && !((i % 3) == 0)) {
                assertEquals("Buzz", result[i - 1]);
            }
        }
    }
}

My resulting implementation became:

public class FizzBuzz {

    private static final int MIN_VALUE = 1;
    private static final int MAX_VALUE = 100;


    private static String[] generate() {
        List<String> items = new ArrayList<String>();

        for (int i = MIN_VALUE; i <= MAX_VALUE; i++) {

            boolean multipleOfThree = ((i % 3) == 0);
            boolean multipleOfFive = ((i % 5) == 0);

            if (multipleOfThree && multipleOfFive) {
                items.add("FizzBuzz");
            }
            else if (multipleOfThree) {
                items.add("Fizz");
            }
            else if (multipleOfFive) {
                items.add("Buzz");
            }
            else {
                items.add(String.valueOf(i));
            }
        }

        return items.toArray(new String[0]);
    }

    public static String[] getResultAsArray() {
        return generate();
    }

    public static String getResultAsString() {
        String[] result = generate();
        String output = "";
        if (result.length > 0) {
            output = Arrays.toString(result);
            // Strip out the brackets from the result
            output = output.substring(1, output.length() - 1);
        }
        return output;
    }

    public static final void main(String[] args) {
        System.out.println(getResultAsString());
    }
}

The whole solution took me around 20 minutes late one evening, including nervously checking over my code for a lot longer than necessary before submitting it :)

Reviewing what I originally submitted: Early on I decided to merge my "multiple of" calculation into the generate() method to avoid over-engineering, which I now think was a mistake; also, the separate getResultAsArray/generate methods were clearly OTT. The getResultAsString could also be merged with the main() method, since one just delegates to the other.

I'm still fairly inexperienced with TDD and I feel this may have let me down in this case. I'm looking for other ways I might have improved on this approach, particularly with regard to TDD practices?

---

Update

Based on the very useful suggestions below, I've reworked my answer to something I now consider would have been more "TDD-friendly":

Changes:

• Separated the FizzBuzz logic from the output generation to make the solution more scalable

• Just one assertion per test, to simplify them

• Only testing the most basic unit of logic in each case

• A final test to confirm the string building is also verified

The code:

public class FizzBuzzTest {

    @Test
    public void testMultipleOfThreeAndFivePrintsFizzBuzz() {
        assertEquals("FizzBuzz", FizzBuzz.getResult(15));
    }

    @Test
    public void testMultipleOfThreeOnlyPrintsFizz() {
        assertEquals("Fizz", FizzBuzz.getResult(93));
    }

    @Test
    public void testMultipleOfFiveOnlyPrintsBuzz() {
        assertEquals("Buzz", FizzBuzz.getResult(10));
    }

    @Test
    public void testInputOfEightPrintsTheNumber() {
        assertEquals("8", FizzBuzz.getResult(8));
    }

    @Test
    public void testOutputOfProgramIsANonEmptyString() {
        String out = FizzBuzz.buildOutput();
        assertNotNull(out);
        assertNotSame(0, out.length());
    }
}

public class FizzBuzz {

    private static final int MIN_VALUE = 1;
    private static final int MAX_VALUE = 100;

    public static String getResult(int input) {
        boolean multipleOfThree = ((input % 3) == 0);
        boolean multipleOfFive = ((input % 5) == 0);

        if (multipleOfThree && multipleOfFive) {
            return "FizzBuzz";
        }
        else if (multipleOfThree) {
            return "Fizz";
        }
        else if (multipleOfFive) {
            return "Buzz";
        }
        return String.valueOf(input);
    }

    public static String buildOutput() {
        StringBuilder output = new StringBuilder();

        for (int i = MIN_VALUE; i <= MAX_VALUE; i++) {
            output.append(getResult(i));

            if (i < MAX_VALUE) {
                output.append(", ");
            }
        }

        return output.toString();
    }

    public static final void main(String[] args) {
        System.out.println(buildOutput());
    }
}

Answer 1

There's a reason why TDD is strongly linked with XP and Agile philosophies. It drives us to small units of testable code. So concepts like TheSimplestThingWhichCouldPossiblyWork or the Single Responsibility Principle fall out of a test driven approach.

That clearly hasn't happened in your scenario. You fixated on the array of numbers, rather than the FizzBuzz bit (the clue really is in the question).

Obviously you're in a totally artificial situation and it's hard to fake TDD. But I would expect "real" TDD code to have exposed the translation methods. Something this:

@Test     
public void testOtherNumber() {        
     String result = FizzBuzz.translateNumber(23);
     assertEquals("23", result);
 } 

@Test     
public void testMultipleOfThree() {        
     String result = FizzBuzz.translateNumber(3);
     assertEquals("Fizz", result);
 } 

@Test     
public void testMultipleOfFive() {        
     String result = FizzBuzz.translateNumber(25);
     assertEquals("Buzz", result);
 } 

@Test     
public void testMultipleOfFifteen() {        
     String result = FizzBuzz.translateNumber(45);
     assertEquals("FizzBuzz", result);
 } 

The point being that each of those produces a clear result and is easy to start with a failing test.

Having done the FizzBuzz bit it is a cinch to do the array stuff. The key point about that is to avoid the hardcoding. Initially we might not want a full implementation: it would be sufficient to generate a relative handful of number of elements, say 15. This has the advantage of producing a better design. After all, if the interviewer came back and said "Actually I want an array of 121 elements", how much of your code would you have to change? How many tests?

---

One of the challenges of TDD is knowing where to start. Gojko Adzic wrote a thought-provoking piece on this, describing a Coding Dojo implementing a game of Go.

---

"Is there a chance exposing my translation methods would have marked against me on the grounds of encapsulation later on?"

One of the most hotly debated topic in TDD. Possible answers are:

1. keep the methods private and embed the unit tests in the class.
2. write the tests against public methods, then make the methods private one the tests pass, and refactor the tests.
3. variation on the above: use conditional compilation (or similar) to expose or hide the methods.
4. just keep them public

There are no right answers, and often it will depend upon specific requirements or personal whim. For instance, while FizzBuzz itself is trivial we are quite often required to write code which takes data, appplies a business rule and returns a validation outcome. Sometimes the rule needs to be applied to a single item of data, sometimes against entire record sets and sometimes against either.

So an API which exposes both methods is not necessarily wrong. And certainly in an interview situation it gives you the opportunity to discuss the nuances of API design, which is a good topic of conversation.

Answer 2

There are two parts to the FizzBuzz puzzle: the loop, and generating the right string for a given int. Traditionally, people combine the two into one function (which is perfectly reasonable, since it's so simple), but for TDD, I would factor out the second part so you can test it independently. In pseudocode:

String[] fizzbuzz(int count)
    for i: 0 ... count:
        line = fizzOrBuzz(i)
        output.add(line)

Now, you can test the fizzOrBuzz method without having to go on the loop, and, confident that it works, you can then test the loop. Make sure you hit possible edge cases (0, -1, Integer.MAX_VALUE).

For something as simple as FizzBuzz, I would cap it there: I wouldn't create a mocked FizzBuzzer and all that. But be prepared to defend that decision (basically by saying the simplicity of the function doesn't warrant a very complex test). When I interview people, I like to suggest a not-great counter-example to an idea of theirs, and see if they can defend their idea (or possibly improve it!).

Answer 3

I won't claim great experience of TDD, so please don't think I'm speaking as an authority! With that in mind, here's my $0.02:

1. I would make all those static methods instance methods, and then create a new FizzBuzz instance for each test.
2. Get rid of generate() and just put that code into getResultAsArray(). (Very minor this.)
3. It's fine to have constants in your unit test class. (i.e. What @APC said).

The other possible changes you mention seem like overkill to me.

One more point: FizzBuzz? Eugh! It's a very poor example question to use since it's so trivial...