Extensible and testable FizzBuzz

Question

There seem no end to fizzbuzz implementations. Something I rarely see is an implementation that's easy to extend with more name-divisor pairs. Another thing that usually annoys me is the lack of testability. Printing to standard output and verifying by reading is less than ideal.

This FizzBuzz class can work with an arbitrary number of fizzers, following these steps:

1. Get a builder with FizzBuzz.builder()
2. Add the fizzers using the builder, for example for classic fizz-buzz:
   • Call .add("Fizz", 3)
   • Call .add("Buzz", 5)
3. Create a FizzBuzz object by calling .build() when ready
4. The .getValue(number) method returns the value for a specified number

The code:

public class FizzBuzz {
    private final List<Fizzer> fizzers;

    private static class Fizzer {
        private final String name;
        private final int divisor;

        private Fizzer(String name, int divisor) {
            this.name = name;
            this.divisor = divisor;
        }
    }

    public static class Builder {
        private final List<Fizzer> fizzers = new ArrayList<>();

        public Builder add(String name, int divisor) {
            fizzers.add(new Fizzer(name, divisor));
            return this;
        }

        public FizzBuzz build() {
            return new FizzBuzz(this);
        }
    }

    private FizzBuzz(Builder builder) {
        fizzers = Collections.unmodifiableList(builder.fizzers);
    }

    public String getValue(int num) {
        String output = "";
        for (Fizzer fizzer : fizzers) {
            if (num % fizzer.divisor == 0) {
                output += fizzer.name;
            }
        }
        return output.isEmpty() ? Integer.toString(num) : output;
    }

    public static Builder builder() {
        return new Builder();
    }
}

A sample demo program using classic fizz-buzz rules, and printing results from 1 to 100:

import static java.util.stream.IntStream.rangeClosed;

class FizzBuzzDemo {
    public static void main(String[] args) {
        FizzBuzz fizzBuzz = FizzBuzz.builder().add("Fizz", 3).add("Buzz", 5).build();
        rangeClosed(1, 100).mapToObj(fizzBuzz::getValue).forEach(System.out::println);
    }
}

Unit tests:

import org.junit.Test;

import static org.junit.Assert.assertEquals;

public class FizzBuzzTest {
    @Test
    public void test_Without_Fizzer() {
        FizzBuzz fizzBuzz = FizzBuzz.builder().build();
        assertEquals("1", fizzBuzz.getValue(1));
        assertEquals("2", fizzBuzz.getValue(2));
        assertEquals("3", fizzBuzz.getValue(3));
        assertEquals("4", fizzBuzz.getValue(4));
        assertEquals("5", fizzBuzz.getValue(5));
        assertEquals("15", fizzBuzz.getValue(15));
        assertEquals("16", fizzBuzz.getValue(16));
        assertEquals(Integer.toString(31 * 3 * 5), fizzBuzz.getValue(31 * 3 * 5));
        assertEquals(Integer.toString(31 * 3 * 5 + 1), fizzBuzz.getValue(31 * 3 * 5 + 1));
    }

    @Test
    public void test_With_Fizz() {
        FizzBuzz fizzBuzz = FizzBuzz.builder().add("Fizz", 3).build();
        assertEquals("1", fizzBuzz.getValue(1));
        assertEquals("2", fizzBuzz.getValue(2));
        assertEquals("Fizz", fizzBuzz.getValue(3));
        assertEquals("4", fizzBuzz.getValue(4));
        assertEquals("5", fizzBuzz.getValue(5));
        assertEquals("Fizz", fizzBuzz.getValue(15));
        assertEquals("16", fizzBuzz.getValue(16));
        assertEquals("Fizz", fizzBuzz.getValue(31 * 3 * 5));
        assertEquals(Integer.toString(31 * 3 * 5 + 1), fizzBuzz.getValue(31 * 3 * 5 + 1));
    }

    @Test
    public void test_With_FizzBuzz() {
        FizzBuzz fizzBuzz = FizzBuzz.builder().add("Fizz", 3).add("Buzz", 5).build();
        assertEquals("1", fizzBuzz.getValue(1));
        assertEquals("2", fizzBuzz.getValue(2));
        assertEquals("Fizz", fizzBuzz.getValue(3));
        assertEquals("4", fizzBuzz.getValue(4));
        assertEquals("Buzz", fizzBuzz.getValue(5));
        assertEquals("FizzBuzz", fizzBuzz.getValue(15));
        assertEquals("16", fizzBuzz.getValue(16));
        assertEquals("FizzBuzz", fizzBuzz.getValue(31 * 3 * 5));
        assertEquals(Integer.toString(31 * 3 * 5 + 1), fizzBuzz.getValue(31 * 3 * 5 + 1));
    }

    @Test
    public void test_With_FizzBuzzJazz() {
        FizzBuzz fizzBuzz = FizzBuzz.builder().add("Fizz", 3).add("Buzz", 5).add("Jazz", 7).build();
        assertEquals("1", fizzBuzz.getValue(1));
        assertEquals("2", fizzBuzz.getValue(2));
        assertEquals("Fizz", fizzBuzz.getValue(3));
        assertEquals("4", fizzBuzz.getValue(4));
        assertEquals("Buzz", fizzBuzz.getValue(5));
        assertEquals("FizzBuzz", fizzBuzz.getValue(15));
        assertEquals("16", fizzBuzz.getValue(16));
        assertEquals("FizzBuzz", fizzBuzz.getValue(31 * 3 * 5));
        assertEquals(Integer.toString(31 * 3 * 5 + 1), fizzBuzz.getValue(31 * 3 * 5 + 1));
        assertEquals("FizzBuzzJazz", fizzBuzz.getValue(31 * 3 * 5 * 7));
    }
}

I'm interested in any kind of improvement you could suggest about any of the above.

Something I don't like is the name Fizzer. Although it's an internal implementation detail not visible to users, I'd like a better, more intuitive name, if somebody can think of one.

As you notice, the constructor of FizzBuzz is forbidden: you can only create it using a builder. The intention behind this is that we need a list of name-divisor pairs, and this seems the most ergonomic option I see in terms of usability. Or maybe there is something else I missed?

Answer 1

You implementation makes assumptions about how Builder.add() is called. If they are not explicitly documented, they are errors in the code.

• Divisors not added in increasing order:

  @Test
  public void BuilderAdd_DifferentDivosorOrder_SameOutput() {
      FizzBuzz fizzBuzz = FizzBuzz.builder().add("Fizz", 3).add("Buzz", 5).build();
      FizzBuzz buzzFizz = FizzBuzz.builder().add("Buzz", 5).add("Fizz", 3).build();
  
      assertEquals(fizzBuzz.getValue(15), buzzFizz.getValue(15));
  }
  

• A divisor is only added once:

  For this case you need to deice how to handle it. Currently both strings will be output. Should the second call take precedence, be ignored, cause an exception?

  @Test
  public void BuilderAdd_DivosorAddedMultipleTimes_EXPECTED_RESULT() {
      FizzBuzz fizzBuzz = FizzBuzz.builder().add("Fizz", 3).add("Buzz", 3).build();
  
      // assert expected result 
  }
  

---

Your test names are fairly generic. When a test fails the first thing you see is the test name. A good test name should be able to give you a good idea of what might be failing even before you see the test. Similarly, when someone is reading the test code for the first time, the shouldn't have to work to get a sense for the intent of the test.

Part of this issue is a result of your test cases being general categories. With this specific setup, try a number of things. Instead it is better to have many specific test cases instead of a few general test cases.

The biggest issue with doing this is that once one thing fails, no other assertions are executed. If there many things broken, you want to know about all of them. What you don't want is to find out that the thing you just fixed was only part of the problem and other things are still failing.

A good test case has three main sections: Arrange, Act, and Assert. In your test cases, you are asserting many unrelated things. Checking a multiple of one is one test. Checking a multiple of both is a different test. Checking a non-multiple of both is yet another test.

When you write your test cases like this, giving specific names becomes much easier. Look at the test case I added to show the error in your code. The name tells you:

• What is being tested.
• The context in which it is being tested.
• The expected result.

If this test fails, I can tell you exactly what is wrong without looking at the code of the test case. Any one seeing the code for the first time doesn't have to think about what was the intent of this specific assertion.

---

Of the test cases you have, then seem to follow that same pattern. For the most part, they only differ on the inputs. Just like with production code, repeated code in tests is a bad thing and can lead to errors.

You can use inheritance to create the core functionality to a set of tests that are similar. An abstract test class can implement a number of test cases. Then you create a subclass for each specific situation. Each subclass can implement a abstract method that produces the FizzBuzz to use in each test case.

For tests when the test cases are testing the same thing, but need to try many different inputs values, you can use parameterized test. This is one good way to avoid the issue of having the first failure prevent other assertions from being executed.

Answer 2

The code in general looks quite good, although it may be slightly over-complicated for the problem it is trying to solve!

You can improve though by using a StringBuilder rather than directly adding two strings together:

public String getValue(int num) {
    String output = "";
    for (Fizzer fizzer : fizzers) {
        if (num % fizzer.divisor == 0) {
            output += fizzer.name;
        }
    }
    return output.isEmpty() ? Integer.toString(num) : output;
}

Which could be simplified to:

public String getValue(int num) {
    String output = fizzers.stream()
        .filter(fizzer -> (num % fizzer.divisor == 0))
        .map(fizzer -> fizzer.name)
        .collect(Collectors.joining());
    return (output.isEmpty() ? Integer.toString(num) : output);
}

By reading this code, two major issues are discovered with the current implemention:

1. A Fizzer does not know if it is divisible.
2. There are no getters on the Fizzer.

By changing Fizzer to the following:

private static class Fizzer {
    private final String name;
    private final int divisor;

    private Fizzer(String name, int divisor) {
        this.name = name;
        this.divisor = divisor;
    }

    private String getName() {
        return name;
    }

    private int getDivisor() {
        return divisor;
    }

    private boolean canDivide(int num) {
        return (num % divisor == 0);
    }
}

By using this, our getValue() can be simplified to the following:

public String getValue(int num) {
    String output = fizzers.stream()
        .filter(fizzer -> fizzer.canDivide(num))
        .map(Fizzer::getName)
        .collect(Collectors.joining());
    return (output.isEmpty() ? Integer.toString(num) : output);
}

Which in turn makes it a lot more readable.

There is even another enhancement we can make, that is we can return a Stream<String> containing the name of the Fizzer if and only if it can divide a number, else we return an empty stream.

private static class Fizzer {
    private final String name;
    private final int divisor;

    private Fizzer(String name, int divisor) {
        this.name = name;
        this.divisor = divisor;
    }

    private String getName() {
        return name;
    }

    private int getDivisor() {
        return divisor;
    }

    private boolean canDivide(int num) {
        return (num % divisor == 0);
    }

    private Stream<String> flatMapIfCanDivide(int num) {
        return (canDivide(num) ? Stream.of(name) : Stream.empty());
    }
}

There should be a better name possible than flatMapIfCanDivide, the getValue method would need to be adopted the following:

public String getValue(int num) {
    String output = fizzers.stream()
        .flatMap(fizzer -> fizzer.flatMapIfCanDivide(num))
        .collect(Collectors.joining());
    return (output.isEmpty() ? Integer.toString(num) : output);
}

Furthermore, you should also be aware that your solution only deals with integers, whereas clients possibly may want to use long or BigInteger, you could consider implementing that. You may even offer to make it generic over T and require a BinaryOperator<T> that can be used to increment T, and a BiPredicate<T, T> that can be used to test if T is divisible by another T.

Answer 3

I see two issues here:

1) The Unit tests have too many asserts

If you have Unit tests that have many scenarios each, each with their own assert, you are doing something wrong.

Each test should test one scenario, so I would suggest using parametrised tests.

This would allow you to set up a battery of tests and a number of units that should be true for each test. Your units would then be more unit-y.

2) Fizz Buzz makes the assumption that Fizzer instances are in numeric order and unique

Why assume?

First make Fizzer implements Comparable<Fizzer>, comparing by divisor. Now stick the whole lot into a TreeSet rather than a List.

Now situations such as those mentioned in unholysampler's answer cannot arise.

Answer 4

If extensible and testable are the key motivators, then here's a simple implementation that fits the bill.

String getValue(int num) {
    StringBuilder builder = new StringBuilder();
    if (num % 3 == 0) {
        builder.append("Fizz");
    }
    if (num % 5 == 0) {
        builder.append("Buzz");
    }
    if (num % 7 == 0) {
        builder.append("Jazz");
    }
    if (builder.length() == 0) {
        builder.append(num);
    }
    return builder.toString();
}

This is easy to extend. If you want to add a new "fizzer" Bang with divisor 13, simply insert 3 lines:

    if (num % 13 == 0) {
        builder.append("Bang");
    }

Where exactly to insert? Wherever it's appropriate for your purpose. In the original code, when you call .add("Bang", 13), you don't really know how that's going to be handled, without reading the interface documentation. Oh wait: the interface documentation is completely missing. So in fact this code is easier to extend.

And this is of course just as testable as the original: the main feature is a function with the exact same signature, so it's fully compatible with the given unit tests, which, as other reviewers have noted, were not properly separated to individual cases.