Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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).

share|improve this question
18  
In my view, any solution to FizzBuzz that doesn't use a simple for loop and System.out.println fails the test. –  RemcoGerlich Jul 12 at 12:53
7  
@RemcoGerlich there's nothing in the FizzBuzz challenge that requires the solution to be imperative as far as I can see. Why do you make that claim? –  Rune FS Jul 12 at 15:16
7  
@Rune FS: It's not in the challenge, but it's just not good programming to make a program much more complex than it needs to be. –  RemcoGerlich Jul 12 at 16:25
2  
@REmcoGerlich using a for loop is more complex that writing this purely functional. –  Rune FS Jul 12 at 17:50
3  
@RuneFS Because you say so? And this is Java, nothing is ever gonna be purely functional in it, so what is the point of your comment? –  Davor Jul 14 at 10:07

8 Answers 8

up vote 11 down vote accepted

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);
}
share|improve this answer
1  
Very good catch on the result.length > 0, I like it! No need for the (stupid) variable anymore. –  skiwi Jul 12 at 12:20
4  
Slightly nicer than result.length > 0 is !result.isEmpty() –  janos Jul 12 at 12:22
    
@janos: I can't say I'm a particular fan of that. While technically a string is a container of chars, it still feels as if it's mainly a basic object and not a datastructure. .isEmpty() is something I associate with collections. –  Jeroen Vannevel Jul 12 at 12:24
4  
@JeroenVannevel in my head the logic goes as "if the string is empty, do this, or else that", and not "if the string has length bigger than 0", so writing it that way feels natural. Incidentally, collections have an isEmpty method too, but that doesn't bother me at all –  janos Jul 12 at 12:31
2  
@JeroenVannevel At least in English, a zero-length string is called an empty string, and isEmpty makes perfect sense. –  David Harkness Jul 12 at 19:18

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).

share|improve this answer
2  
I like this one, I was actually experimenting with this whole creating the current solution. I'll be sure to make a little library class to hold the different options and I'll create a followup using this! –  skiwi Jul 12 at 12:36
1  
It would be better if the classes turned into humble methods...java 8, you know. Less enterprise-y. –  Michael Deardeuff Jul 13 at 5:03
    
I don't think Java has foreach. –  Simon Kuang Jul 23 at 19:00
    
@SimonKuang: "note that this isn't intended to be compilable Java, just Java-like pseudocode". In a nearly C++-like avoidance of introducing new keywords, Java just extended the for syntax to provide foreach-like semantics. –  Jerry Coffin Jul 24 at 14:37

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.

share|improve this answer
    
At least to me, this seems a bit too clever and far too easy to use in ways that look like they should work, but will actually fail completely. An interesting solution nonetheless. –  Jerry Coffin Jul 12 at 16:38
    
@Jerry Coffin Could you please explain? I'm used to functional programming (Scala), so this is not "clever" at all to me. And what about "look like they should work"? This code gave me the right answer right away. I did not have to do any debugging. –  toto2 Jul 12 at 16:44
1  
@Jerry Coffin And by the way, my code is nearly identical to yours, but terser since I'm using Java 8. Function is your Substitute and function composition (andThen) is nearly equivalent to your Mapper. –  toto2 Jul 12 at 16:47
1  
I'm talking about the fact that overWriter uses its own internal counter, so if (for example) you tried to get a sequence starting from another number (e.g., 25) instead of 1, the output would be entirely wrong. –  Jerry Coffin Jul 12 at 17:08
    
Yeah, having four independently incrementing counters is a code smell here. –  Ben Voigt Jul 13 at 16:35

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));
    }
}
share|improve this answer
    
The compiler will not convert it to a StringBuilder as the concatenation is done on separate lines. It will instead, actually create multiple string objects along the way. I agree with your suggestion about unit tests though, but your claim that the compiler will convert it to StringBuilder is not entirely accurate. –  Simon André Forsberg Jul 12 at 18:41
    
Good point. I dropped it from my answer, thanks for pointing out! –  janos Jul 12 at 18:47

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.

share|improve this answer
5  
The customer is equally likely to tell you that Buzz should change to Bugz, and expect the FizzBuzz print to change accordingly. You're way overthinking this. –  RemcoGerlich Jul 12 at 12:50
2  
I think everyone over thinks FizzBuzz @RemcoGerlich. That why I say it's important to be able to explain your thought process; whatever it may be. –  ckuhn203 Jul 12 at 13:11

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.

share|improve this answer

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);
}
share|improve this answer

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.

share|improve this answer
    
Thanks, I added some bits about my motivation for writing these exact pieces of code. I'd like to know what is not particularly good about them, if it's not too much trouble. –  HS_Tri Jul 14 at 10:29
    
If you'd like to have your code reviewed, you should post your own question. –  ckuhn203 Jul 14 at 10:35
    
Thanks again, I addressed all your remarks, hopefully. I'm still going to assume that "FizzBuzz" is meant as a concatenation of "Fizz" and "Buzz", but I might be wrong! –  HS_Tri Jul 14 at 11:01

protected by Simon André Forsberg Jul 14 at 9:39

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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