4
\$\begingroup\$

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);
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ Note for potential reviewers: Both handlers have almost identical code, which doesn't look DRY to me. Can this be fixed by passing 3 and 5 as arguments instead of having these magic numbers in an override? \$\endgroup\$
    – Mast
    Aug 11, 2022 at 8:31
  • \$\begingroup\$ @Mast TL;DR yes that would make totally sense! Detail: maybe for theses two handler: yes you could have a super class for that , a fraction Handler! as OpenClosed i was thinking of adding a prime number handler as well, that handler would not match into the category of a FractionHandler \$\endgroup\$ Aug 11, 2022 at 9:04
  • 1
    \$\begingroup\$ Please include the specification to the question. FizzBuzz may be common knowledge, but it's such entry level assignment that I don't keep the spec "in cache." :) \$\endgroup\$ Aug 11, 2022 at 9:59
  • \$\begingroup\$ @TorbenPutkonen ok, thats so obvious and still i didn^t see it :-( i will add tehm to my question very soon! thanks for pointing this out (haha - fun fact: we have even a tag for fizzbuzz) \$\endgroup\$ Aug 11, 2022 at 10:47

2 Answers 2

3
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ that are some real issues, thank you very much for your review (even fizzbuzz seems simply, it opens so much possibilities on how to improve ones way of coding) \$\endgroup\$ Aug 11, 2022 at 10:49
3
\$\begingroup\$

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).

\$\endgroup\$
1
  • \$\begingroup\$ thank you very much, it´s good to see another view on the code who agrees on (at least) some ideas! And thanks for concering the order as another important point. (I went on holiday and had therefore some delay on answering) Thanks a lot! \$\endgroup\$ Aug 16, 2022 at 5:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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