StackOverflow2013

Note that there are some explanatory texts on larger screens.

plurals

POWays to improve my FizzBuzz solution for a TDD role?
text
Body
copied!<p>I had an interview recently where I was asked to produce the traditional FizzBuzz solution:</p> <blockquote> <p>Output a list of numbers from 1 to 100.</p> <ul> <li>For all multiples of 3 and 5, the number is replaced with "FizzBuzz"</li> <li>For all remaining multiples of 3, the number is replaced with "Fizz"</li> <li>For all remaining multiples of 5, the number is replaced with "Buzz"</li> </ul> </blockquote> <p>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:</p> <pre><code>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]); } } } } </code></pre> <p>My resulting implementation became:</p> <pre><code>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()); } } </code></pre> <p>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 :)</p> <p>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.</p> <p>I'm still fairly inexperienced with TDD and I feel this may have let me down in this case. <strong>I'm looking for other ways I might have improved on this approach, particularly with regard to TDD practices?</strong></p> <hr> <h2>Update</h2> <p>Based on the very useful suggestions below, I've reworked my answer to something I now consider would have been more "TDD-friendly":</p> <p>Changes: </p> <ul> <li><p>Separated the FizzBuzz logic from the output generation to make the solution more scalable</p></li> <li><p>Just one assertion per test, to simplify them</p></li> <li><p>Only testing the most basic unit of logic in each case</p></li> <li><p>A final test to confirm the string building is also verified</p></li> </ul> <p>The code:</p> <pre><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()); } } </code></pre>

Querying!

Guidance

An individual column

Larger individual text columns get their own page to allow for proper reading.

SQuiL has stopped working due to an internal error.

If you are curious you may find further information in the browser console, which is accessible through the devtools (F12).

Reload