Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 a 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());
    }
}

Self-examining 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

  • 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.

  • @APC on StackOverflow pointed out that my approach is not scalable, perhaps I should have better separated the logic from the string building?

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?

This is cross-referenced from my SO question, since you can't move SO questions to Code Review, and someone pointed out that this is a better forum for this type of request. In retrospect, I should have come here in the first place!


Based on the very useful suggestions on StackOverflow, I've reworked my answer to something I now consider would have been more "TDD-friendly". This is my second attempt at a solution:

  • 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());
    }
}
share|improve this question
    
@Winston: Thanks for the correction, this is probably better - I was originally thinking others would vote up/down my draft revision –  seanhodges Mar 8 '12 at 9:05

2 Answers 2

up vote 4 down vote accepted
public class FizzBuzzTest {

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

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

This doesn't feel like a TDD test. If you wrote this in a test-first manner I'd expect you to start with 3, not 93. Not that testing 93 is bad, but I'd expect a test for the trivial cases as well.

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

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

I'd expect more tests in general. I'd probably consistently test the first 15 numbers. I'd store the correct results in an array and use a loop to assert accuracy in each case.

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

This test seems inefficient. You check for non-null, non-empty strings, but have no checks to make sure the actual output made any sense.

}

public class FizzBuzz {

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

I'd make these parameters to buildOutput not class constants.

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

I think you can probably get rid of some of those parentheses.

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

I'd put this in an else, I think it gives a clearer idea of what you are doing and looks more parallel with the rest of the function

    }


    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());
    }
}
share|improve this answer
    
An excellent answer, the references to TDD convention were just what I was looking for. It looks like you would recommend something in-between to 2 approaches I made for my tests: DO have multiple assertions in the test (in the form of a loop), but DON'T let the test contain logic (and use a hand crafted array instead). –  seanhodges Mar 8 '12 at 9:15
    
I expected the actual output of of my testOutputOfProgramIsANonEmptyString() had been already tested by my other tests (beyond the building of a String) - but it does make sense to ensure we aren't just printing gobbledygook. –  seanhodges Mar 8 '12 at 9:15
    
@seanhodges, if your testing framework supports it, use data-driven tests rather then the loop I mentioned. Then each item in the array counts as its own test rather then as one giant test. I strongly discourage any sort of logic in the test. Logic is what we get wrong in programs, and we need our tests to be correct. If nothing else, you've never tested your comma logic. –  Winston Ewert Mar 8 '12 at 17:19
  • You can avoid the special case for FizzBuzz
  • No need to hard-code the limits
  • It's not really bad if you have multiple returns in a short (!) method, but it should be avoided if you have an equally readable solution with only one return

I would write it that way:

public class FizzBuzz {

    public static String getResult(int input) {
        String result = "";
        if (input % 3 == 0) {
            result = "Fizz";
        }
        if (input % 5 == 0) {
            result += "Buzz";
        }
        return result.isEmpty() ? "" + input : result;
    }

    public static String buildOutput(int from, int to) {
        StringBuilder output = new StringBuilder();
        for (int i = from; i <= to; i++) {
            if(output.length() > 0) {
                output.append(", ");
            }
            output.append(getResult(i));
        }
        return output.toString();
    }

    public static final void main(String[] args) {
        System.out.println(buildOutput(1, 100));
    }
}
share|improve this answer
    
+1 for getResult –  Dan Jan 6 at 18:28

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.