3
\$\begingroup\$

Okay code reviewers, I need you to make my FizzBuzz code even better. Any suggestions for improvement, optimizations, etc.

public final class FizzBuzz {

    private FizzBuzz() {};

    /**
     * comparisons for a multiple of 3 = 2
     * comparisons for a multiple of 5 = 2
     * comparisons for a multiple of 15 = 2
     * 
     */
    public static void fizzBuzzBad() {
        // most common thought process
        for (int n = 1; n < 20; n++) {
            if (n % 3 == 0) {
                System.out.print("fizz");
            } else if (n % 5 == 0) {
                System.out.println("buzz");
            } else {
                System.out.println("\n" + n);
            }
        }
    }

    /**
     * comparisons for a multiple of 3 = 2
     * comparisons for a multiple of 5 = 2
     * comparisons for a multiple of 15 = 1 <--- here is the savings.
     * 
     * This saves the number of comparison for 15's.
     */
    public static void fizzBuzzBetter() {
        // most common thought process
        for (int n = 1; n < 20; n++) {
            if (n % 15 == 0) {
                System.out.print("fizzbuzz");
            } else if (n % 3 == 0) {
                System.out.print("buzz");
            } else if (n % 5 == 0) {
                System.out.println("fizz");
            } else {
                System.out.println("\n" + n);
            }
        }
    }

    public static void main(String[] args) {
        fizzBuzzBad();
        System.out.println("--------------------------------------------");
        fizzBuzzBetter();
    }

}
\$\endgroup\$
4
\$\begingroup\$
  1. Your methods should take the loop limit as a parameter.
  2. You should separate computation from output. There are several options:
    • Pass in a StreamWriter you can write to
    • Pass in an interface with a Write method or similar.
    • Return a list of the results to completely remove the output concern.
    • Implement the Enumeration<T> interface to return the results which will completely remove the output concern from the class while not having to temporarily buffer them as with the list solution.
  3. fizzBuzzBad is broken due to the use of else if.
  4. You miss most of the line breaks in fizzBuzzBetter
\$\endgroup\$
4
\$\begingroup\$

In both versions, your policy on printing newlines is inconsistent. With fizzBuzzBetter(), all of your cases are mutually exclusive, so they should all be System.out.println(...) calls, and there is no need to prepend "\n".

Your fizzBuzzBad() is wrong: it won't print "fizzbuzz" for multiples of 15.

Anything that can easily be parameterized ought to be. Instead of hard-coding 20, your functions should take a max parameter that is passed from main.

\$\endgroup\$

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.