4
\$\begingroup\$

Task: Create the Fizz Buzz game using Test Driven Developoment. Print the correct word for range 1-100.

The implementation:

public class Main {

    static final String FIZZ = "Fizz";
    static final String BUZZ = "Buzz";
    static final String FIZZ_BUZZ = "Fizz Buzz";

    public static void main(String[] args) {
        IntStream.range(1, 101).forEach((x) -> System.out.println(getWordForNumber(x)));
    }

    public static String getWordForNumber(int x) {
        if (isDivisibleWithoutRemainder(x, 15)) {
            return FIZZ_BUZZ;
        } else if(isDivisibleWithoutRemainder(x, 3)) {
            return FIZZ;
        } else if(isDivisibleWithoutRemainder(x, 5)) {
            return BUZZ;
        }
        return Integer.toString(x);
    }

    private static  boolean isDivisibleWithoutRemainder(int dividend, int divisor) {
        return (dividend%divisor == 0);
    }

}

The JUnit Test:

final class Number {
    private final int n;

    Number (int number) {
      n = number;
    }

    final boolean isMatchedToWord(final String word) {
      return Main.getWordForNumber(n).equals(word);
    }
}

class MainTest {
    @Test
    public void numbersAreMatchedToWords_Works() {
      assertTrue(new Number(1).isMatchedToWord("1"));
      assertTrue(new Number(2).isMatchedToWord("2"));
      assertTrue(new Number(3).isMatchedToWord("Fizz"));
      assertTrue(new Number(4).isMatchedToWord("4"));
      assertTrue(new Number(5).isMatchedToWord("Buzz"));
      assertTrue(new Number(6).isMatchedToWord("Fizz"));
      assertTrue(new Number(10).isMatchedToWord("Buzz"));
      assertTrue(new Number(15).isMatchedToWord("Fizz Buzz"));
      assertTrue(new Number(30).isMatchedToWord("Fizz Buzz"));
    }
}
\$\endgroup\$
4
\$\begingroup\$
  • Prefer IntStream.rangeClosed(1, 100) instead of IntStream.range(1, 101)
  • Be consistent in spacing, if (isDivisibleWithoutRemainder(x, 3)) is better than if(isDivisibleWithoutRemainder(x, 3)) (note the space after if)
  • return (dividend%divisor == 0); does not need the parenthesis, I would also recommend to give it a bit more space. return dividend % divisor == 0;
  • Use assertEquals in your test instead of assertTrue to show a better error message if the test fails
  • You could write your own helping method to test a number instead of having the Number class, such as testNumber(4, "4"); which basically does assertEquals(word, Main.getWordForNumber(n));
  • Don't test all cases in the same method. If one goes wrong you need to fix it before you can see the results of the others below it. Preferably use a JUnit parameterized test
\$\endgroup\$
  • \$\begingroup\$ IntStream.rangeClosed is definitive better. I didn't know about the parameterized tests; they seem very useful - ty \$\endgroup\$ – Matthias Herrmann May 11 '18 at 10:58
5
\$\begingroup\$

Test driven development

In test driven development you let the code and the tests evolve by repeating the following steps:

  1. Add a new test case that fails, the simplest possible
  2. Fix the implementation the simplest possible way so that all test cases pass
  3. Optionally refactor the existing code, while keeping all tests passing
  4. Repeat from step 1

There are variations to these steps, but one thing is certain: in the end you will have many test cases. The posted code has one, which means it wasn't really developed using TDD.

Keep it simple

It's good to keep everything as simple as possible. In the test class, do you really need a Number class to wrap an number, so that you create an instance and then call a method on that instance? A simple method would have been enough, that takes an integer as parameter and returns a string.

\$\endgroup\$
  • \$\begingroup\$ What you see above is the result of me applying test driven development. I wrote always a unit test that has failed before writing enough and not a single line more so that the test succeeded. Each assert statement is one unit test that initially failed (so I got 9x iterations). To the second part: Yes you are right the Number class isn't the best solution. I wanted that someone who is reading the test can simply read number x is being matched to word y. In C++ I could just overload the operator() but in Java that doesn't work, so I had to create an instance.... \$\endgroup\$ – Matthias Herrmann May 11 '18 at 11:54
  • \$\begingroup\$ I didn't write 9 test methods because of the DRY principle. But as @SimonForsberg pointed out parameterized tests are a possible solution for this issue. \$\endgroup\$ – Matthias Herrmann May 11 '18 at 12:01
  • 3
    \$\begingroup\$ @MatthiasHerrmann Fair enough about the iterations, but there really has to be distinct method (= test case), not just a new assertion in an existing one. The unit of growth is the test case. It's best when the new test method has name that describes its purpose. It takes some practice to come up with good names. \$\endgroup\$ – janos May 11 '18 at 12:25
  • 2
    \$\begingroup\$ @MatthiasHerrmann as for fluent assertions, I recommend using the assertj library. You will be able to write like assertThat(getWordForNumber(3)).isEqualTo("Fizz");, which I think comes very close to what you wanted to achieve. Even without that, assertEquals(getWordForNumber(3), "Fizz") would have been better than the current solution. \$\endgroup\$ – janos May 11 '18 at 12:27
  • 1
    \$\begingroup\$ @janos Don't forget that expected comes before actual, so that'd be assertEquals("Fizz", getWordForNumber(3)) \$\endgroup\$ – Simon Forsberg May 11 '18 at 16:29
3
\$\begingroup\$

FIZZ = "Fizz" really? I'm amazed no one has mentioned that naming variables after their values will earn you a special place in the after life. Don't couple names to values.

Most of what's been said here I agree with but another issue that's been ignored is the code for 15.

Try adding a requirement to print "Bazz" on multiples of 7 and I think you'll see why I'm taking issue with branching off of 15.

If you don't, here's a well thought out blog post on Fizz Buzz that delves into the 15 issue called The Wrong FizzBuzz. It almost shows you a better way to solve this problem. Unfortunately it makes a fatal mistake and changes the requirements to make it's prefered solution easier.

The problem with hard coding 15 is it's an invitation for new requirements to cause a combinatorial explosion of code. There is a simple way around it. Just treat your string like the collection of letters it is and add to it.

public static String getWordForNumber(int x) {

    String result = "";

    if ( isDivisibleBy(x, 3) ) {
        result += "Fizz";
    } 

    if ( isDivisibleBy(x, 5) ) {
        result += "Buzz";
    }

    if ( result.equals("") ) {
        result = Integer.toString(x);
    }

    return result;
}

Now yes, just like the blog this doesn't quite work. Darn spaces are messing it up. But it only needs three little changes. Easy fix.

public static String getWordForNumber(int x) {

    String result = "";

    if ( isDivisibleBy(x, 3) ) {
        result += " Fizz";
    } 

    if ( isDivisibleBy(x, 5) ) {
        result += " Buzz";
    }

    if ( result.equals("") ) {
        result = Integer.toString(x);
    }

    return result.trim();
}

Now it passes all the tests and the only numbers that will ever need to be mentioned are prime.

\$\endgroup\$
  • \$\begingroup\$ There's two different (reasonable) possible requirements that can happen: A) Add "Bazz" on multiples of 7. B) Print "Quack" for multiples of 15 instead of "FizzBuzz". It's hard to build and be prepared for both of those scenarios. \$\endgroup\$ – Simon Forsberg May 14 '18 at 15:18
  • \$\begingroup\$ @SimonForsberg I don't see how that forces an explosion of code. Just start withif ( isDivisibleBy(x, 15) ) return "Quack"; and the rest still works. The pattern is to treat prime numbers and compound numbers differently. \$\endgroup\$ – candied_orange May 14 '18 at 18:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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