The FizzBuzz challenge in Java 8 written in a short, readable and interesting way

Question

I decided to take on the FizzBuzz challenge with as twist that I would use Java 8 concepts to make it a bit modular, yet still let it be a short, readable and understandable program.

This in contrary to some gem I found on the net: FizzBuzzEnterpriseEdition

The problem description:

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"

Here's my code:

public class FizzBuzz {
    private static Stream<String> fizzBuzz(final int min, final int max) {
        if (min < 0) {
            throw new IllegalArgumentException("min is negative: min = " + min);
        }
        if (min > max) {
            throw new IllegalArgumentException("min > max: min = " + min + " / max = " + max);
        }
        return IntStream.rangeClosed(min, max)
                .mapToObj(FizzBuzz::fizzBuzzify);
    }

    private static String fizzBuzzify(final int value) {
        StringBuilder stringBuilder = new StringBuilder();
        boolean toDefault = true;
        if (value % 3 == 0) {
            stringBuilder.append("Fizz");
            toDefault = false;
        }
        if (value % 5 == 0) {
            stringBuilder.append("Buzz");
            toDefault = false;
        }
        return (toDefault) ? String.valueOf(value) : stringBuilder.toString();
    }

    public static void main(String[] args) {
        fizzBuzz(1, 100).forEach(System.out::println);
    }
}

I'm still looking for a nicer way to write fizzBuzzify, my intention however is to not hardcode the if (value % 15 == 0) similarly if (value % 3 == 0 && value % 5 == 0), because it creates a sort of illogical operation precedence, being that you absolutely need to write the if (value % 15 == 0) case up front, followed by the 3-case and the 5-case (or vica versa).

Answer 1

A guideline that I use for deciding on a StringBuilder or not is that it depends on whether I know beforehand how many times it will be used or not. I believe I've come across this recommendation on some MSDN page once but I'm not entirely sure.

In your case, you know that there are only two possible uses of your StringBuilder, and that there are no values added inside a loop, so I would use normal string concatenation:

Now you can also change your code a little to what I consider more easily interpretable:

private static String fizzBuzzify(final int value) {
   String result = "";

   if (value % 3 == 0) {
       result += "Fizz";
   }
   if (value % 5 == 0) {
       result += "Buzz";
   }
   return result.length() > 0 ? result : Integer.toString(value);
}

Answer 2

I think if you're going to do this, it's better to separate out the conditions. I don't know Java 8 well enough to use it, but in older Java, I'd consider something like this (note that this isn't intended to be compilable Java, just Java-like pseudocode):

interface Substitute {
    Boolean condition(int);
    String transform(int);
}

class Fizzer : implements Substitute {
    Boolean condition(int x) { return x % 3 == 0; }
    String transform(int) { return "fizz"; }
};

class Buzzer : implements Substitute {
    Boolean condition(int x) { return x % 5 == 0; }
    String transform(int) { return "buzz"; }
};

class Mapper {
    List<Substitute> subs;

    void add_sub(Substitute sub) {
        subs.Add(sub);
    }    

    String execute(int input) { 
        String result;
        Boolean use_default = true;

        foreach (sub : subs) {
            if (sub.condition(input)) {
                result += sub.transform(input);
                use_default = false;
            }
        }
        if (use_default) return String.valueOf(input);
        return result;
    }
}

class FizzBuzz {
    static void main() { 
        Mapper map;
        map.add_sub(new Fizzer);
        map.add_sub(new Buzzer);

        for (int i=0; i<100; i++)
           System.out.println(map.execute(i));
    }
}

This separates out the basic concept of "if some condition is met, substitute a string for the number" from the individual conditions and results produced. Like yours, however, it still doesn't entirely avoid depending on ordering to some degree (and I don't think such dependency can be avoided). If you don't maintain ordering of the substitutions, you could end up replacing a multiple of 15 with "buzzfizz" instead of the required "fizzbuzz".

To avoid that, you have a couple of choices. One is to maintain an ordered list of the mapping objects, and run all of them in order for every input. If two or more fire, you concatenate the results together, and it's up to the client code to order appropriately.

Alternatively, you can stop at the first one that "fires" (i.e., the first one for which the "condition" returns true). This requires that the client code include a "FizzBuzzer" that will "fire" for multiples of 15.

Personally, I'd prefer to use that, and extend it a bit further. Instead of the ugly conditional code in the loop that decides whether a transformation has already happened, or to use the default transformation (i.e., just printing out the number), I'd prefer to just add a default transformation to the end of the list of transformations:

class DefaultMap : implements Substitute {
    Boolean condition(int) { return true; }
    String transform(int val) { return String.valueOf(val); }
};

// ... in main:
Mapper map;

map.add_sub(new FizzBuzzer);
map.add_sub(new Fizzer);
map.add_sub(new Buzzer);
map.add_sub(new DefaultMap);

for (int i=0; i<100; i++)
    System.out.println(map.execute(i));

As far as the code itself goes, the way I've written it is fairly verbose. Although I don't know Java 8 well enough to actually write the code to do it, I feel fairly confident that it would allow the substitutions to be written as lambda expressions, which should substantially reduce verbosity without loss of generality (and given my ignorance of Java 8, I'd almost be surprised if it didn't allow other improvements as well).

Answer 3

For a trivially simple use case like this, StringBuilder is overkill. Use simple concatenation instead:

String result = "";
if (value % 3 == 0) {
    result = "Fizz";
}
if (value % 5 == 0) {
    result += "Buzz";
}

The brackets around toDefault here are unnecessary:

return (toDefault) ? String.valueOf(value) : stringBuilder.toString();

How about a convenience generator with only a max argument, using min=1 by default:

static Stream<String> fizzBuzz(final int max) {
    return fizzBuzz(1, max);
}

How about adding some unit tests:

public class FizzBuzzTest {
    private String resultToString(int from, int to) {
        return FizzBuzz.fizzBuzz(from, to).collect(Collectors.joining(" "));
    }

    @Test
    public void testShortRanges() {
        assertEquals("1 2 Fizz 4 Buzz Fizz", resultToString(1, 6));
        assertEquals("7 8 Fizz Buzz 11 Fizz 13 14 FizzBuzz", resultToString(7, 15));
    }

    @Test
    public void testConsistentConcat() {
        String a = resultToString(1, 20);
        String b = resultToString(21, 100);
        assertEquals(a + " " + b, resultToString(1, 100));
    }
}

Answer 4

I'm going to quote this answer as you've made the same mistake in interpreting the requirements. Although, I'm not sure how many interviewers would catch the difference. You should be prepared to explain why it's better to use value % 3 == 0 && value % 5 ==0 over the solution you've implemented or over hard coding the value of 15.

Write a program that prints the integers 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"

However you have implemented this:

Write a program that prints the integers 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 the concatenation of both.

Why does it matter? If you consider the problem as the business logic given provided by a customer then approx. 5sec after the deployment of your solution the customer will come back and say: "Ah yes, I forgot, if it's divisible by 3 and 5 you have to print FixBugz because some of our legacy applications which we can't change have a typo in their parsing code." Now instead of just changing array(3 => 'Fizz', 5 => 'Buzz', 15 => 'FizzBuzz') into array(3 => 'Fizz', 5 => 'Buzz', 15 => 'FixBugz') you have to change a whole bunch of implementation code and unit tests.

I'm not very good with Java, but I like this answer's implementation, so I'm going to make a slight improvement to it. Instead of hard coding all those magic numbers, FizzBuzzify should take some parameters. I explain this fully in this answer to a different FizzBuzz question.

private static String fizzBuzzify(final int value, final int fizzDivisor, final int buzzDivisor) {
  if (value % fizzDivisor == 0) {
    return (value % buzzDivisor == 0) ? "FizzBuzz" : "Fizz";  
  }
  return (value % buzzDivisor == 0) ? "Buzz" : Integer.toString(value);
}

I would recommend making fizzDivisor and buzzDivisor optional parameters with default values though. I just don't know how that's done in Java.

Answer 5

Except for the StringBuilder, I agree with your solution.

I wrote a completely different solution which I think it is more "functional". It might not be the most efficient solution, but this code is reusable for different problems.

/**
 * @return a function that simply returns its input value, except that it 
 *         returns {@code overWriteValue} each time the function as been
 *         called a multiple of {@code periodicity} times.
 */
public static <T> Function<T, T> overWriter(int periodicity, T overWriteValue) {
    AtomicInteger counter = new AtomicInteger(0);
    return value -> (counter.getAndIncrement() % periodicity) == 0  
                        ? overWriteValue 
                        : value;
}

public static void main(String[] args) {
    Stream<String> ints = IntStream.range(0, 100).mapToObj(Integer::toString);
    Function<String, String> fizzBuzzOverWriter = 
            overWriter(3, "Fizz")
            .andThen(overWriter(5, "Buzz")
            .andThen(overWriter(15, "FizzBuzz")));
    Stream<String> fizzBuzz = ints.map(fizzBuzzOverwriter);
    fizzBuzz.forEach(System.out::println);
}

Note that I am not at all using the value of the integers when choosing the overwrite value. You have to make sure the start value of the Stream and the initial counter value are in some agreement. (I could also have added the initial counter value as an argument to overWrite().)

I could also have made my overWrite function: Integer -> String. But the code is then less reusable, if at all. Also, composing the three different overlapping conditions would not be as simple.

Answer 6

I personally think it's better to test for divisibility by the second term in the first if (and you did not forbid the ternary) so,

private static String fizzBuzzify(final int value) {
  if (value % 3 == 0) {
    return (value % 5 == 0) ? "FizzBuzz" : "Fizz";  
  }
  return (value % 5 == 0) ? "Buzz" : Integer.toString(value);
}

This way there are no extra temporaries in the method.

Answer 7

I would go with an enum to define the substitiutions, something like:

private static enum Transformer implements IntFunction<Optional<String>> {
    FIZZ {
        @Override
        public Optional<String> apply(int value) {
            return value % 3 == 0 ? Optional.of("Fizz") : Optional.empty();
        }
    },
    BUZZ {
        @Override
        public Optional<String> apply(int value) {
            return value % 5 == 0 ? Optional.of("Buzz") : Optional.empty();
        }
    };
}

So now you can easily add new "transformations" to the list.

In order to transform an int to the correct String in Scala we would simple use flatMap as Option is also a collection. No such luck in Java. I came up with this thing, it's a little ugly but I think it's clear:

private static String transform(final int i) {
    final StringBuilder sb = Stream.of(Transformer.values()).
            map(t -> t.apply(i)).
            collect(StringBuilder::new, (builder, v) -> v.ifPresent(builder::append), StringBuilder::append);
    if (sb.length() == 0) {
        sb.append(i);
    }
    return sb.toString();
}

Essentially we take each transformer and we apply it that gives us a Stream<Optional<String>>. I then you the collect method to append all present instances to a StringBuilder. If the `StringBuilder is empty simply append the current integer too.

An alternative, not sure if it's better, is:

private static String transform(final int i) {
    return Stream.of(Transformer.values()).
            map(t -> t.apply(i)).
            filter(Optional::isPresent).
            map(Optional::get).
            collect(collectingAndThen(joining(), s -> s.isEmpty() ? Integer.toString(i) : s));
}

Here we do the map to get the Stream<Optional<String>>, we then filter out absent and map to a Stream<String>. We then use the (often overlooked) collectingAndThen Collector to first join the resulting Stream<String> and then to return i if the joined String is empty.

The main work now becomes as simple as:

public static void main(String[] args) throws Exception {
    IntStream.rangeClosed(1, 100).mapToObj(App::transform).forEach(System.out::println);
}

Answer 8

We can further enhance the code in Jeroen Vannevel's answer (which doesn't use StringBuilder when it isn't helping, and removes an unneeded boolean variable) with the following further improvements:

• eliminate any string concatenation and generation of garbage strings, and
• eliminate need for additional boolean or method calls to length() or isEmpty() as some other answers suggest.

The result is:

private static String fizzBuzzify(final int value) {
   String result = null;

   if (value % 3 == 0) {
       result = "Fizz";
   }
   if (value % 5 == 0) {
       result = (result == null) ? "Buzz" : "FizzBuzz";
   }
   return (result != null) ? result : Integer.toString(value);
}

My Java 8-ified answer uses mapToObj() to transform the output without any redundant checks or added booleans by recognizing 15 as the common factor of 3 and 5 so the conditions can be written in clear discreet steps. I don't see the need to complicate this with extra methods, so there is simply one lambda expression to map the values, and a method reference to System.out.println for the output I/O to the forEach() method:

    public static void fizzBuzz() {
        IntStream.rangeClosed(1, 100)
                .mapToObj(n -> (n%15 == 0) ? "FizzBuzz"
                              : (n%3 == 0) ? "Fizz"
                              : (n%5 == 0) ? "Buzz"
                              : Integer.toString(n))
                .forEach(System.out::println);
    }

Answer 9

It says "print", so I'll use sysout. Both solutions are IMHO also relatively short and readable.

I assume that "FizzBuzz" is meant as a concatenation of "Fizz" and "Buzz". That might not be true, but it's nicer here, I think.

The first one uses uses the Consumer-Interface which was added to Java in Version 8.

class FizzBuzzConsumer implements IntConsumer {

    @Override
    public void accept(int value) {
        if (value % 3 == 0) {
            System.out.print("Fuzz");
        }
        if (value % 5 == 0) {
            System.out.print("Buzz");
        }
        if (value % 3 != 0 && value % 5 != 0) {
            System.out.print(value);
        }
        System.out.println();
    }
}

The FizzBuzzConsumer takes an int-Value and decides what to print. It does not care whether the int-Value is a multiple of both 3 and 5, because that condition can be fulfilled by each separately.

public class FizzBuzz {

    public static void main(String[] args) {
        FizzBuzzConsumer fizzBuzzConsumer = new FizzBuzzConsumer();

        IntStream.rangeClosed(1, 100)
            .forEach(fizzBuzzConsumer);
    }
}

This created a stream of integers, each of which is fed into the FizzBuzzConsumer.

Another solution which simply looks at each int in the stream (peek) to decide whether to print Fizz and/or Buzz or the value itself:

public class FizzBuzz2 {

    public static void main(String[] args) {
        IntStream.rangeClosed(1, 100)
            .peek(possibleMultipleOfThree -> {
                if (possibleMultipleOfThree % 3 == 0) {
                    System.out.print("Fizz");
                }
            })
            .peek(possibleMultipleOfFive -> {
                if (possibleMultipleOfFive % 5 == 0) {
                    System.out.print("Buzz");
                }
            })
            .peek(possibleMultipleOfNeither -> {
                if (possibleMultipleOfNeither % 3 != 0 && possibleMultipleOfNeither % 5 != 0) {
                    System.out.print(value3);
                }
            })
            .forEach(intValue -> System.out.println());
    }
}

Again this creates a stream of integers. It looks ("peeks") at each of them to decide whether what to print, followed by a linebreak. It's more or less the same as above, but much more verbose.

Neither seems particularly interesting to me, though. In both cases I felt that repeating the comparison of the current int-value for the opposite condition (if (value % 3 != 0 && value % 5 != 0)) would make for the highest readability.