5
\$\begingroup\$

I made this FizzBuzz game for my college class and it's kind of ridiculous, but is suppose to demonstrate chain of command so writing it in a little forloop was out of the question. In case you don't know the game, this program counts numbers 1-100 printing them to the console but replace any multiple of 3 with the word Fizz, multiples of 5 with the word Buzz, and multiples of 3 and 5 with the word FizzBuzz. There's actually more I'd like to do to it, but I've already been "spoken to" about adding extra features that he didn't ask for so I'm trying to keep this simple. I'll take any critique though because I tried new things like declaring string as constants and not using curly braces when my if statements only had one line. I have another version with user inputs and the ability to restart, and I'm thinking about making it go backwards, use letters, and add options for other multiples.

public class Main {
    public static void main(String[] args) {

        int startNum = 1;
        int endNum = 100;
        
        GameMgr.Start(startNum, endNum);
    }
}
public abstract class GameMgr {

    // THESE ARE THE MESSAGES THAT WILL BE PRINTED
    // IF THE CURRENT NUMBER IS DIVISIBLE WITH THE
    // CURRENT OBJECTS DIVISOR
    final String FIFTEEN = "FizzBuzz";
    final String THREE = "Fizz";
    final String FIVE = "Buzz";

    // THESE HOLD THE CURRENT NUMBER BEING
    // EVALUATED AS WELL AS THE NUMBER THAT
    // INDICATES THE END
    static int currNum;
    static int endNum;

    //DEFAULT CONSTRUCTOR
    public GameMgr(){}

    // FILLS THE VALUES TO BE EVALUATED AND
    // STARTS THE CHAIN OF COMMAND
    public static void Start(int a, int b){

        currNum = a;
        endNum = b;
        new BuzzFizz();
    }

    // CALCULATES THE CURRENT VALUE WITH THE
    // CURRENT OBJECTS DIVISOR
    protected boolean isDivisible(int divisor){
        return currNum % divisor == 0;
    }

    // CHECKS IF THE SYSTEM HAS EXCEEDED THE
    // ENDPOINT. IF NOT, VALUE IS INCREASED
    // AND CHAIN OF COMMAND IS RESTARTED
    protected void ProcessNext(){
        currNum++;
        if (currNum <= endNum)
            GameMgr.Start(currNum, endNum);

        else
            System.exit(0);
    }

    // METHOD IS IMPLEMENTED IN THE CHILD CLASSES
    // TO EVALUATE THE CURRENT WITH WITH THEIR
    // RESPECTIVE DIVISOR
    public abstract String Eval();
}
public class BuzzFizz extends GameMgr {

    // SPECIFIC DIVISOR FOR THIS CHILD
    int divisor = 15;

    // CALLS THE EVALUATION METHOD DETERMINES
    // WHETHER TO CONTINUE CHAIN OR RESTART
    // THE CHAIN
    public BuzzFizz(){

        System.out.println(Eval());
        if (Eval() != null)
            ProcessNext();
    }

    // CHECKS IF THE CURRENT NUMBER IS DIVISIBLE
    // WITH THIS OBJECTS DIVISOR RETURNS MESSAGE
    // TO PRINT TO CONSOLE IF TRUE, CREATES
    // NEXT OBJECT IN THE CHAIN IF FALSE
    @Override
    public String Eval(){

        if (isDivisible(divisor))
            return FIFTEEN;

        new Fizz();
        return "";
    }
}
public class Fizz extends GameMgr {

    // SPECIFIC DIVISOR FOR THIS CHILD
    int divisor = 3;

    // CALLS THE EVALUATION METHOD DETERMINES
    // WHETHER TO CONTINUE CHAIN OR RESTART
    // THE CHAIN
    public Fizz(){

        System.out.println(Eval());
        if (Eval() != null)
            ProcessNext();
    }

    // CHECKS IF THE CURRENT NUMBER IS DIVISIBLE
    // WITH THIS OBJECTS DIVISOR RETURNS MESSAGE
    // TO PRINT TO CONSOLE IF TRUE, CREATES
    // NEXT OBJECT IN THE CHAIN IF FALSE
    @Override
    public String Eval(){

        if (isDivisible(divisor))
            return THREE;

        new Buzz();
        return "";
    }
}
public class Buzz extends GameMgr {

    // SPECIFIC DIVISOR FOR THIS CHILD
    int divisor = 5;

    // CALLS THE EVALUATION METHOD DETERMINES
    // WHETHER TO CONTINUE CHAIN OR RESTART
    // THE CHAIN
    public Buzz(){

        System.out.println(Eval());
        if (Eval() != null)
            ProcessNext();
    }

    // CHECKS IF THE CURRENT NUMBER IS DIVISIBLE
    // WITH THIS OBJECTS DIVISOR RETURNS MESSAGE
    // TO PRINT TO CONSOLE IF TRUE, CREATES
    // NEXT OBJECT IN THE CHAIN IF FALSE
    @Override
    public String Eval(){
        if (isDivisible(divisor))
            return FIVE;

        new BasicNum();
        return "";
    }
}
public class BasicNum extends GameMgr {

    // PRINTS THE CURRENT NUMBER AND
    // RESTARTS THE CHAIN
    public BasicNum(){

        System.out.println(currNum);
        ProcessNext();
    }

    // SINCE THIS IS THE END OF THE
    // CHAIN, THIS METHOD DOESN'T NEED
    // TO DO ANYTHING
    @Override
    public String Eval(){
        return null;
    }
}
\$\endgroup\$
3
\$\begingroup\$

In no particular order:

In Java, constants must be final. Otherwise they can be reassigned, making them not, er, constant. :)

In Java, methods always start with a lowercase letter.

You're calling eval() twice in each constructor, rather than performing the computation once and remembering the value in a variable.

Abstract class methods should almost always be either abstract (part of the contract) or final (part of the implementation). You probably don't want subclasses able to redefine what processNext() does.

Your're defining the FizzBuzz Strings in GameMgr, but the divisors in the subclasses. The Strings should be private instance variables in the subclasses with the divisors.

There's no reason for currNum and endNum to be static.

Optional curly braces aren't. They should always be used to improve readability and prevent errors.

In Java there is whitespace between a ) and a {.

Most of your abbreviations hurt readability. It's helpful to spell out words unless there's a compelling reason to abbreviate them.

The divisor instance variables should be private. Always make everything as tightly scoped as you can.

GameMgr trying to both loop through all the values and being a parent of the implementations is a violation of the Single Responsibility Principle. Classes should have one job.

Why is your class called "BuzzFizz" instead of "FizzBuzz"?

Your design decision to do all the work in the constructors is questionable. You're not constructing meaningful objects, but rather using the constructors to perform business logic that would be better suited for a method. This implementation choice also suggests that you haven't fully grokked how a Chain of Responsibility is supposed to work. The idea is to create a list of objects, where each object may either handle an input or pass it on to the next link in the chain. The chain of objects exists as a reusable entity.

So, let's try to do better:

public class Main {

    public static void main(String[] args) {
        AbstractHandler buzzFizz = new BuzzFizz();
        AbstractHandler fizz = new Fizz();
        AbstractHandler buzz = new Buzz();
        AbstractHandler basicNumber = new BasicNumber();

        buzzFizz.setNext(fizz);
        fizz.setNext(buzz);
        buzz.setNext(basicNumber);

        /* now we have a chain buzzFizz -> fizz -> buzz -> basicNumber
         * we can reuse this chain later, or pass it to another method
         * by sending the head of the chain as an argument, like
         * doSomethingWith(buzzFiz). That other code can call
         * buzzFizz.handle() and it will go down the chain, even though it
         * only sees the head.
         */             

        for (int i = 1; i <= 100; i++) {
            buzzFizz.handle(i);
        }
    }
}

.

public abstract class AbstractHandler {

    private final int divisor;
    private AbstractHandler next;

    public AbstractHandler(int divisor) {
        this.divisor = divisor;
    }

    public final void setNext(AbstractHandler next) {
        this.next = next;
    }

    public final void handle(int value) {
        if ((value % divisor) == 0) {
            System.out.println(displayValue(value));
            return;
        }
        if (next != null) {
            next.handle(value);
        }
    }

    protected abstract String displayValue(int value);

}

.

public class BuzzFizz extends AbstractHandler {

    public BuzzFizz() {
        super(15);
    }

    @Override
    protected String displayValue(int value) {
        return "FizzBuzz";
    }

}

.

public class BasicNumber extends AbstractHandler {

    public BasicNumber() {
        super(1);
    }

    @Override
    protected String displayValue(int value) {
        return Integer.toString(value);
    }

}

So this is better, but still not great. We'd really like to separate out our public API into an interface, and keep the implementation details to ourselves. If we did that, and we notice that the only difference between the FizzBuzz, Fizz, and Buzz handlers is the numbers, we can cut it down to two implementations:

public class Main {

    public static void main(String[] args) {
        Handler buzzFizz = new FizzBuzzHandler(15, "FizzBuzz");
        Handler fizz = new FizzBuzzHandler(3, "Fizz");
        Handler buzz = new FizzBuzzHandler(5, "Buzz");
        Handler basicNumber = new BasicNumberHandler();

        buzzFizz.setNext(fizz);
        fizz.setNext(buzz);
        buzz.setNext(basicNumber);

        for (int i = 1; i <= 100; i++) {
            buzzFizz.handle(i);
        }
    }
}

.

public interface Handler {

    void setNext(Handler next);

    void handle(int value);
}

.

public final class FizzBuzzHandler implements Handler {

    private final int divisor;
    private final String displayValue;
    private Handler next;

    public FizzBuzzHandler(int divisor, String displayValue) {
        this.divisor = divisor;
        this.displayValue = displayValue;
    }

    @Override
    public final void setNext(Handler next) {
        this.next = next;
    }

    @Override
    public final void handle(int value) {
        if ((value % divisor) == 0) {
            System.out.println(displayValue);
            return;
        }
        if (next != null) {
            next.handle(value);
        }
    }

}

.

public final class BasicNumberHandler implements Handler {

    public BasicNumberHandler() {
        super();
    }

    @Override
    public void setNext(Handler next) {
        throw new UnsupportedOperationException("This is a terminal handler.");
    }

    @Override
    public void handle(int value) {
        System.out.println(value);
    }

}

Can we do better? We can, if we use a little black magic. Java allows us to pass methods as a type of variable. We can get away with just one class which gets the acceptance test and the output injected into it:

public class Main {

    public static void main(String[] args) {
        Handler buzzFizz = new Handler(value -> value % 15 == 0, value -> System.out.println("FizzBuzz"));
        Handler fizz = new Handler(value -> value % 3 == 0, value -> System.out.println("Fizz"));
        Handler buzz = new Handler(value -> value % 5 == 0, value -> System.out.println("Buzz"));
        Handler basicNumber = new Handler(value -> true, value -> System.out.println(value));

        buzzFizz.setNext(fizz);
        fizz.setNext(buzz);
        buzz.setNext(basicNumber);

        for (int i = 1; i <= 100; i++) {
            buzzFizz.handle(i);
        }
    }
}

.

import java.util.function.Consumer;
import java.util.function.Predicate;

public final class Handler {

    private final Predicate<Integer> predicate;
    private final Consumer<Integer> consumer;

    private Handler next;

    public Handler(Predicate<Integer> predicate, Consumer<Integer> consumer) {
        this.predicate = predicate;
        this.consumer = consumer;
    }

    public final void setNext(Handler next) {
        this.next = next;
    }

    public final void handle(int value) {
        if (predicate.test(value)) {
            consumer.accept(value);
            return;
        }

        if (next != null) {
            next.handle(value);
        }
    }

}

This is a chain with exactly one type of link. It might be preferable to keep the idea of an interface Handler with one implementation type, just in case you needed to add a new implementation type later.

\$\endgroup\$
1
  • \$\begingroup\$ I'm not going to go back and edit the code, but note that it would have been much more correct to have handle() return the String to be displayed rather than printing it. \$\endgroup\$
    – Eric Stein
    Dec 7 '20 at 18:08
2
\$\begingroup\$

I may come back with a more detailed critique, but some initial thoughts are

  1. Don't do significant processing in your constructors. It can be done, but goes against many developers' expectations.
  2. Don't use global data. Pass arguments.
  3. I don't think BUZZ etc should be subclasses of GameMgr. They don't seem to me to be the same sorts of things.
  4. BUZZ etc hold no real state, so could be singletons, or enum members.

Update: I said I may come back...

I'm also concerned that the BUZZ etc classes have more than one responsibility - I don't think they should deal with incrementing the number...

I'd call BUZZ etc "handlers" - they should have a single method "handle(int)" and either return the appropriate string or pass the integer on.

I'm not sure we need as much in these classes as eric's answer suggests, and I'm far from convinced we need closures...

Here's a quick and fairly short enum-based solution.

public class FBChain {
  static enum Handler {
    FIZZBUZZ(15), BUZZ(5), FIZZ(3), //

    PLAIN_NUMBER(0) {
      // Override the method for this special-case
      @Override
      protected String handle(int candidate) {
        return Integer.toString(candidate);
      }
    };
    
    final int divisor;
    Handler nextHandler = null;

    Handler(int divisor) {
      this.divisor = divisor;
    }

    protected String handle(int candidate) {
      if ((candidate % divisor) == 0) {
        return name();
      }
      return nextHandler.handle(candidate);
    }

    static {
      FIZZBUZZ.nextHandler = BUZZ;
      BUZZ.nextHandler = FIZZ;
      FIZZ.nextHandler = PLAIN_NUMBER;
    }

    public static String chainHandle(int candidate) {
      return FIZZBUZZ.handle(candidate);
    }
  }

  public static void main(String[] args) {
    for (int candidate = 1; candidate <= 50; candidate++) {
      System.out.println(Handler.chainHandle(candidate));
    }
  }
}
\$\endgroup\$
2
  • \$\begingroup\$ The problem with baking the chain into the enum is that now all clients must use the same chain. Letting clients define and order their own links is a more flexible approach. \$\endgroup\$
    – Eric Stein
    Dec 7 '20 at 18:10
  • \$\begingroup\$ @EricStein I can see your point of view, but I follow the You Ain't Gonna Need It principle, and the task in front of me was to implement FizzBuzz using a Chain of Command pattern, not implement a generalised Chain of Command framework. I like that the whole definition of FizzBuzz is in the one class, rather than having a generalised handler which is then "customised" elsewhere, but it's a difference of philosophies and tastes, I'd say. (Also I'm conditioned to older Java and don't find closures natural yet :-) \$\endgroup\$ Dec 8 '20 at 8:26

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.