# Ways to improve my coding test FizzBuzz solution for a TDD role?

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) {
}
else if (multipleOfThree) {
}
else if (multipleOfFive) {
}
else {
}
}

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

-
@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

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

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

-
+1 for getResult –  Dan Jan 6 '14 at 18:28
"" + input while this works, it's really not obvious what it's doing. Wouldn't using a method from String make more sense? –  raptortech97 Aug 7 '14 at 19:11
@raptortech97 It's a very common idiom in Java every Java developer should know, so why not use it? –  Landei Aug 8 '14 at 21:46