I'm trying to learn streams and hashmaps, in doing so I want to make a very scalable, very clean version of FizzBuzz while adhering to the book Clean Code by Robert Cecil Martin.

public class Main {

    public static void fizzBuzzHundredTimes(Map<String, Integer> fizzDivisor) throws ArithmeticException{
        String output; // Avoid recreating String each iteration
        for (int i = 1; i <= 100; i++) {
            final int finalI = i; // Variables used in lambda functions need to be final.
            output = fizzDivisor.entrySet()
                    .filter(entry -> finalI % entry.getValue() == 0)
                    .map(entry -> entry.getKey())
            if (output.length() > 0) {
                // the value was divisible.
            } else {



    public static void main(String[] args) {

        Map<String, Integer> fizzMap = new HashMap<>();
            // Avoid negative numbers
        } catch(Exception e){
            System.err.println(e + "Was thrown");


3 Answers 3

  • You can use output.isEmpty() instead of checking its length.
  • You might want to use a LinkedHashMap which presists the order of the elements as well. Right now the ordering will be seemingly random (Will it be BuzzFizzFuzzBiffBizz? Or FizzFuzzBiffBizzBuzz?) (technically it's deterministic, but it's based on the hashcodes of the strings, which is seemingly random)
  • Your exception handling System.err.println(e + "Was thrown"); doesn't tell you anything about why the exception happened. I would recommend using a logger (Slf4j / Log4j) and log the exception properly, or use e.printStackTrace() to make it easier for you to debug the problems.

    In your final version however, you should not need any exception handling as all the bugs that could cause them to appear should have been fixed - as this is an application based on pure logic and no network calls and stuff.

  • String output; // Avoid recreating String each iteration technically, the string is recreated each iteration anyway. There's nothing you can do to avoid that.

    You're only declaring it once. It's best practice to declare it in a small scope as possible, so I would recommend declaring it only when you initialize it.

  • You could create another method to return a single string for a single number, instead of having one method to process all the 100 numbers.
  • // Avoid negative numbers I don't see how that comment is relevant at the location it is written. Negative numbers are prevented by the for-loop in your code.

In order to be more readable, i would split the code into these three functions:

public static void fizzBuzzNTimes(Map<String, Integer> fizzDivisor, int times) {
    .mapToObj(n-> ifEmpty( fizzBuzzForN(n,fizzDivisor), String.valueOf(n)) )

private static String fizzBuzzForN(int n, Map<String,Integer> fizzDivisor) {
   return fizzDivisor
      .filter(entry -> n % entry.getValue() == 0)

private static String ifEmpty (String value, String defaultValue) { 
    return value.isEmpty() ? defaultValue : value; 


on the topic of Java 8 streams, You can replace the for loop with IntStream which emits a stream of integer values.

to emit a stream of values between 1 and 100 inclusive:

IntStream.rangeClosed(1, 100)
    .foreach(i -> {

Your Answer

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

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