15
\$\begingroup\$

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?

\$\endgroup\$
  • 3
    \$\begingroup\$ The instrumentation that you have to add to make FizzBuzz unit-testable makes it unbearably worse than the standard straightforward solution. You would be much better off with readable code that is correct by inspection. \$\endgroup\$ – 200_success Dec 22 '14 at 18:32
  • 1
    \$\begingroup\$ @200_success for the record, I strongly disagree. The only thing in this code to make it testable is the getValue function that returns a string instead of printing it. I don't see what's unbearable about that. \$\endgroup\$ – janos Dec 22 '14 at 18:49
  • 2
    \$\begingroup\$ This implementation is overkill? I once implemented FizzBuzz with Windows Workflow Foundation... \$\endgroup\$ – Roger Lipscombe Dec 22 '14 at 20:05
  • 1
    \$\begingroup\$ Extensible FizzBuzz? Ha! Even my Brainfuck implementation was that! ;) \$\endgroup\$ – Simon Forsberg Dec 22 '14 at 22:38
  • 1
    \$\begingroup\$ Still not as extensible and testable as FizzBuzz Enterprise Edition :) \$\endgroup\$ – user11153 Dec 23 '14 at 14:11
15
\$\begingroup\$

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.

\$\endgroup\$
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 1. A Fizzer does not know if it is divisible. Why would it? Having the divisor is enough to find that out, just like having access to the name is enough to determine if the name matches a regex or is empty. You wouldn't hoist the entire Math or String classes unto Fizzer just because it gives you access to a number and a string. \$\endgroup\$ – Doval Dec 22 '14 at 22:18
  • 1
    \$\begingroup\$ Do you reckon returning a Stream is better than returning an Optional? I know that Optional is not monadic in Java 8, but it just makes more sense to me. \$\endgroup\$ – Boris the Spider Dec 23 '14 at 8:17
  • \$\begingroup\$ @Doval Because it's the only operation you care about here, so you are not hosting an entire library. It also provides cleaner code. \$\endgroup\$ – skiwi Dec 23 '14 at 10:10
  • 1
    \$\begingroup\$ @BoristheSpider I've thought about that, but I don't think you can integrate it in a stream this nicely, but it might very well be a better solution as this one might feel a bit weird. \$\endgroup\$ – skiwi Dec 23 '14 at 10:11
  • 1
    \$\begingroup\$ @skiwi yes, Optional should certainly implements Set to ease integration. You can always filter(Optional::isPresent).map(Optional::get) but you're right - it's messier. \$\endgroup\$ – Boris the Spider Dec 23 '14 at 11:31
5
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ You'll never make Senior Architect with an attitude like that ;) Kidding aside, I can't help wondering if Poe's Law applies here. \$\endgroup\$ – user62054 Dec 30 '14 at 0:45
  • \$\begingroup\$ It totally applies! Or does it really? \$\endgroup\$ – janos Dec 30 '14 at 8:51

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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