yet another fizz buzz

Question

FizzBuzz is a well known Exercise among programmers, but I wanted to add an aspect called "openClosed" principle (from SOLID)

FizzBuzz is a very simple programming task, used in software developer job interviews, to determine whether the job candidate can actually write code. It was invented by Imran Ghory, and popularized by Jeff Atwood. Here is a description of the task:

Write a program that prints the numbers from 1 to 100. But for multiples of three print “Fizz” instead of the number and for the multiples of five print “Buzz”. For numbers which are multiples of both three and five print “FizzBuzz”.

App for running an example

public class App{

    public static void main(String[] args) {
        FizzBuzz fizzBuzz = new FizzBuzz();
        fizzBuzz.addHandler(new FizzHandler());
        fizzBuzz.addHandler(new BuzzHandler());
        fizzBuzz.print(100);
    }
}

Handler

public interface Handler {

    boolean matches(int number);

    String getMessage();
}

FizzHandler an Handler implementation

public class FizzHandler implements Handler {

    @Override
    public boolean matches(int number) {
        return number % 3 == 0;
    }

    @Override
    public String getMessage() {
        return "Fizz";
    }
}

BuzzHandler an Handler implementation

public class BuzzHandler implements Handler {
    @Override
    public boolean matches(int number) {
        return number % 5 == 0;
    }

    @Override
    public String getMessage() {
        return "Buzz";
    }
}

FizzBuzz

public class FizzBuzz {

    private List<Handler> handlers = new ArrayList<>();
    public void print(int countDestination) {
        IntStream.range(0,countDestination).forEach(this::printFizzBuzz);
    }

    private void printFizzBuzz(int i) {
        List<Handler> matchingHandler = handlers.stream().filter(h -> h.matches(i)).toList();
        String output = matchingHandler.isEmpty()?
                Integer.toString(i):
                matchingHandler.stream().map(Handler::getMessage).collect(Collectors.joining());
        System.out.println(output);
    }

    public void addHandler(Handler handler) {
        handlers.add(handler);
    }
}

Answer 1

The handlers combine matching and processing into the same class. Two responsibilities in one module violates single responsibility principle. To me the handlers look like unnecessary decoration for Predicate<Integer> and Supplier<String>. Combining them together, behind a specialized interface, makes reuse and change more complicated.

The FizzBuzz class combines the looping functionality and the handler matching. Again two responsibilities. It relies on the fact that the user must register the handlers in a specific order, which cannot be deduced from anything in the API. I would rather see the handlers be associated with a priority or some other way where their execution order is made clear in the registration code. I personally associate handler registration with a map. I don't know if other share this but changing the method name to addLast(Handler) would already make it clear that there is an order. The API does not make it clear that the class also relies on both handlers being executed when they both match. I don't have an API change ready that would rectify this so at least it should be documented in the JavaDoc.

The print(int countDestination) gives no clues about the parameter actually defining a range. It should be renamed to printRange(int start, int end). It should also include error checking for start being greater than end. I'm not sure if printing a range should be a feature at all. The FizzBuzz class could just be a Function<Integer, String> so it could be included in stream operations elsewhere. After all, an IntStream is much more versatile than always forcing the user into a "zero to something" range.

The handlers always output to System.out which closes the main method to change. There is no way to write the output to, for example, a network socket. The FizzBuzz probably should just produce the data and let some other component decide where it gets written.

Answer 2

I do agree with @TorbenPutkonen about the FizzBuzz class responsibilities and API:

• Its responsibility should just be knowing how to apply the handlers to produce a String from an int. The range logic and the presentation should be left to its client code.

• The addHandler method name doesn't tell about execution order. If this was going to be extended with lots of handlers, probably the priority idea would be great. For a simpler case, a name like appendHandler would be fine to me.

Regarding the Handler interface, though it has two responsibilities, I would not split it into two interfaces. That would leave the responsibility of knowing the core logic to the code setting up FizzBuzz. The Handlers encapsulate that logic behing a meaningful name and I think that's great. It keeps together the logic that runs together.

If we really wanted to reuse the processing and matching logic separately from each other, I would keep the Handler class, and have it delegate to a Predicate<Integer>. That way, we would maintain the FizzHandler and BuzzHandler, and they would delegate their matching to a MultipleMatcher (maybe it could have some other name. That's the best I could currently come up with).