General FizzBuzz in Java

Question

I have this generalization of FizzBuzz in Java:

import java.util.Map;
import java.util.TreeMap;

public class GeneralFizzBuzz {

    public static void generalFizzBuzz(Map<Integer, String> map, 
                                       int start, 
                                       int stop) {
        String startString = String.valueOf(start);
        String stopString = String.valueOf(stop);

        // Find the width which can accommodate any line number:
        int lineNumberWidth = Math.max(startString.length(), 
                                       stopString.length());

        // Create the format string for printing line numbers:
        String lineNumberFormat = "%" + lineNumberWidth + "d: ";

        for (int i = start; i <= stop; ++i) {
            // Print the line number and the colon with a space:
            System.out.printf(lineNumberFormat, i);

            for (Map.Entry<Integer, String> entry : map.entrySet()) {
                if (i % entry.getKey() == 0) {
                    System.out.print(entry.getValue());
                }
            }

            System.out.println();
        }
    }

    public static void main(String[] args) {
        Map<Integer, String> map = new TreeMap<>();

        map.put(2, "Foo");
        map.put(7, "Bar");
        map.put(11, "Baz");

        generalFizzBuzz(map, -200, 200);
    }

}

As always, please tell me whatever comes to mind.

Answer 1

Hmm... the generalFizzBuzz method is a mix of business logic and output, I think this should rather be separeted.

Thus, I'd rather have a business logic conversion number, which converts a given number to test to the output string:

private static String convertNumberForOutput(Map<Integer, String> numberReplacement, int num) {
    // not necessarily better than a loop, but to show a different approach
    // (attn: I have not tried to compile this ;-))
    return numberReplacement.entrySet().stream()
        .filter(e -> (num % e.getKey()) == 0)
        .map(Map.Entry::getValue)
        .collect(Collectors.joining());
}

In the generalFizzBuzz method, replace the inner loop with a call to that function.

Naming: "map" is a no-go-name. This is like calling start and stop "numberA" and "numberB". The map has a logical purpose, so you should reflect this in its naming.

Answer 2

Comments

Comments should only be used to tell why the code is like it is.

Your comments are different, they somewhat split the method into parts. Therefore you should rather create methods named after the comments:

public static void generalFizzBuzz(Map<Integer, String> map, 
                                   int start, 
                                   int stop) {
    String startString = String.valueOf(start);
    String stopString = String.valueOf(stop);
    int lineNumberWidth = findWidthAccommodatingAnyLineNumber(startString, stopString);
    String lineNumberFormat = createFormatStringForLineNumbers(lineNumberWidth);
    for (int i = start; i <= stop; ++i) {
        printLineNumberUsing(lineNumberFormat, i);
        for (Map.Entry<Integer, String> entry : map.entrySet()) {
            if (i % entry.getKey() == 0) {
                System.out.print(entry.getValue());
            }
        }
        System.out.println();
    }
}
private static void printLineNumberUsing(String lineNumberFormat, int i){         
    System.out.printf(lineNumberFormat, i);
}
private static int findWidthAccommodatingAnyLineNumber(String startString, STring stopString){
    return Math.max(startString.length(), stopString.length());
}
private static String createFormatStringForLineNumbers(int lineNumberWidth){
     return "%" + lineNumberWidth + "d: ";
}

Naming

Your method is called generalFizzBuzz which is not starting with a verb. Methods schould always start with a verb or at least express an action.

Visibility scopes

Your method has the public scope modifier. But since there is currently no other class using it it should have private visibility scope.

Correctness

Your code fails to solve the game because the rules are to output either current number or the word, not both.

Answer 3

It is all static.

It's not bad per se, but why not use the notion of Object ?

You could make GeneralFizzBuzz an Object that holds parameterizable the rules, and presents a method to apply those rules to a range. As it is, you've put the user on a position where he/she has to know that you represent the fizzbuzz rules as a Map. This is unnecessary.

Have a look at this stateful FizzBuzz (without changing anything else):

public class GeneralFizzBuzz {

    private final Map<Integer, String> map = new HashMap<>();

    public void addRule(int targetNumber, String output){
        map.put(targetNumber, output);
    }

    public void fizzBuzz(int start, int stop) {
        String startString = String.valueOf(start);
        String stopString = String.valueOf(stop);

        // Find the width which can accommodate any line number:
        int lineNumberWidth = Math.max(startString.length(), 
                                       stopString.length());

        // Create the format string for printing line numbers:
        String lineNumberFormat = "%" + lineNumberWidth + "d: ";

        for (int i = start; i <= stop; ++i) {
            // Print the line number and the colon with a space:
            System.out.printf(lineNumberFormat, i);

            for (Map.Entry<Integer, String> entry : map.entrySet()) {
                if (i % entry.getKey() == 0) {
                    System.out.print(entry.getValue());
                }
            }

            System.out.println();
        }
    }

    public static void main(String[] args) {
        GeneralFizzBuzz gfb = new GeneralFizzBuzz();

        gfb.addRule(2, "Foo");
        gfb.addRule(7, "Bar");
        gfb.addRule(11, "Baz");

        gfb.gizzBuzz(-200, 200);
    }
}

I think this is easier to use, less prone to mistakes, etc.