Which FizzBuzz is better, and why?

Question

I was in an interview and the guy asked me to do the typical FizzBuzz question of printing numbers 1-100, but for each factor of 3 print Fizz, each factor of 5 print Buzz, and each factor of both print FizzBuzz. I wrote the following:

public static void fizzbuzz(){
    for(int i=1;i<=100;i++){
        if(i%15 == 0){
            System.out.println("FizzBuzz");
        } else if (i%3 == 0) {
            System.out.println("Fizz");
        } else if (i%5 == 0) {
            System.out.println("Buzz");
        } else {
            System.out.println(i);
        }
    }
}

The interviewer had me change my answer so that it looked like this:

public static void fizzbuzz(){
    for(int i=1;i<=100;i++){
        String str = "";
        if(i%3 == 0){
            str = "Fizz";
        }
        if(i%5 == 0){
            str += "Buzz";
        }
        if(str.equals("")){
            str = i;
        }
        System.out.println(str);
    }
}

Then he asked me which I would prefer if I was handed that code, and said there was no wrong answer (which I should have known was a lie), but I foolishly said mine because it was easier for me to look and see that something different was happening for factors of 15 in my code by just glancing over it, whereas without actually looping through the code in my mind, I wouldn't know that about the second version.

Anyways, my question is, is his version better for some reason that I am missing? I'm at entry level skill, so I honestly think that both versions are completely identical efficiency and readability wise.

Answer 1

I prefer your solution to his. My reasoning is that:

• yours has constants only in the String manipulation
• you do not do string concatenation
• the logic is distinct for each remainder.

Both options could be better though. The lack of 'breathing space' (white space between keywords, variables, values, and operators) leads to hard-to-read code. Lines like:

for(int i=1;i<=100;i++){

should be:

for (int i = 1; i <= 100; i++) {

Then, your solution would actually be more efficient as a switch statement:

for (int i = 1; i <= 100; i++) {
    switch (i % 15) {
        case 0:
            System.out.println("FizzBuzz");
            break;
        case 3:
        case 6:
        case 9:
        case 12:
            System.out.println("Fizz");
            break;
        case 5:
        case 10:
            System.out.println("Buzz");
            break;
        default:
            System.out.println(i);
    }
}

The switch statement will be optimized so that there does not need to be more than one modulo, and there is no concept of an if/elseif/elsif/else... just a single condition check.

Answer 2

Then he asked me which I would prefer if I was handed that code

Here's what I would say: It depends.

rolfl has already added some comments about your solution so I'll add some comments (pros and cons) about the second solution.

First of all, rolfl's comment about spacing applies here too, of course.

The biggest pro about his solution is that it's more flexible/extensible. If you wanted to add Woof, Ping, Plop you could easily do so without having to write a whole lot of code.

However, the cons:

• It's using String (instead of StringBuilder) which creates a lot of more objects.
• It does not compile as str = i; is an illegal assignment (a string cannot be set to an int). You'd have to use str = String.valueOf(i); or Integer.toString(i);
• It is much better to use if (str.isEmpty()) rather than if (str.equals(""))
• Instead of setting the string to an integer if it's empty, use System.out.println(i); or System.out.println(str); to gain some speed. (As you won't have to create a string for the integer)

So in the end, it all depends on what you want. If you want to trade-off speed for flexibility then the second version is better (once you correct the above points).

Looking only at the speed though, your original version is better (especially after rolfl's suggestion to use a switch, even though that reduces the maintainability of the code a little bit).

It's not all black and white, there's a lot of pros and cons here. I would bring up those pros and cons when being asked that question, then ask the interviewer: "What's the probability that we need to add Woof, Ping, Plop later on?" (Note: I have not yet answered like this at an interview)

Answer 3

This is a review of the second one - I think the first one is perfectly acceptable, the second one I've got issues.

Strings

There's a string in there that is printed out at the end. Ok. However, whenever the integer is divisible by 5, the string is being changed with the instruction: str += "Buzz";

This constructs a new String object each time through that if block which is completely inappropriate for a String. If it is to be mutable, make it a StringBuilder instead.

I know people will go "but this will only hit it 20 times - are 20 new objects that big a deal?" And the answer is "no, 20 new objets aren't a big deal - but this is indicative of someone who will put it in a loop that gets executed a few thousand times and wonder where the memory is going" (I've fixed that code before).

String comparison

Setting aside the += for the String a bit, the final if statement in that block:

    if(str.equals("")){
        str = i;
    }

We're doing a test of a string against a String with .equals(""). Because of String interning, this "" is exactly the same object as the "" at the start of the loop.

This means that the test could be done as:

    if(str == ""){
        str = i;
    }

I'll make the disclaimer now that while this is correct and would be faster, static analysis tools will complain about it and people looking at it will think its a bug. You probably shouldn't do it that way... And its going to do that test anyways in the first test of .equals(Object anObject) in the String class.

The grumble that I have there is more one of that if the String is not "", then there is a lot more tests that need to be done before it returns back to false.

A test of:

    if(str.length() == 0){
        str = i;
    }

or

    if(str.isEmpty()){
        str = i;
    }

is a better, more succinct test that says exactly what it means without going through all the hoopla of the code in .equals().

---

There seems to be some confusion about my stance on String tests.

    if(str == ""){
        str = i;
    }

I said that that was correct... and it is in that for this code it will give the correct answer all of the time. It is not good code. It will cause static analysis tools to raise flags about it - for good reason. It will cause your co-workers to look at it and think you've got a bug there - for good reason. It is not good code.

Doing a .equals("") in Java is excessive. One of the static analysis inspections that IntelliJ has under "Performance Issues" is String.equals("")

Reports .equals() being called to compare a String with an empty string. It is normally more performant to test a String for emptiness by comparing its .length() to zero instead.

Thus, the options that should be considered for this are instead:

    if(str.length() == 0){
        str = i;
    }

or

    if(str.isEmpty()){
        str = i;
    }

(I'd go for the isEmpty() over length() == 0 though at the end of the day its the same).

So, why is this an issue? Lets look at the code for .equals(Object anObject) (the code is slightly different in 6-b14 than 7u40-b43 through 8-b132 though it is essentially the same).

1012    public boolean equals(Object anObject) {
1013        if (this == anObject) {
1014            return true;
1015        }
1016        if (anObject instanceof String) {
1017            String anotherString = (String)anObject;
1018            int n = count;
1019            if (n == anotherString.count) {
1020                char v1[] = value;
1021                char v2[] = anotherString.value;
1022                int i = offset;
1023                int j = anotherString.offset;
1024                while (n-- != 0) {
1025                    if (v1[i++] != v2[j++])
1026                        return false;
1027                }
1028                return true;
1029            }
1030        }
1031        return false;
1032    }

If they are the same object (this will be true for the case where "" falls through because neither i%3 nor i%5 is the case, it will kick out right at the front and we're done. However, if the String is "Fizz" or "Buzz" or "FizzBuzz" it will then check to make sure its an instance of the String then test the count (a private final value set up in the constructor).

The count for "" is 0, while the other strings are either 4 or 8, and so then we will fall through to the return on 1031 with a false.

So why the big deal?

Well, the code for isEmpty() is:

670     public boolean isEmpty() {
671         return count == 0;
672     }

Really short and to the point. The shortness of the method call matters. One of those -XX calls that no one ever touches (nor should they without knowing what they are doing) is: -XX:MaxInlineSize=# (Java HotSpot VM Options). The default for this is 35 bytes of byte code.

If you have a small method, HotSpot will inline the method. No stack frame, no method lookup after HotSpot decides its going to do something. Its just smack right there in the code.

    if(str.isEmpty()){
        str = i;
    }

becomes something that looks more like

    if(str.count == 0){
        str = i;
    }

This is why people say that lots of small methods in Java is a good thing - because the compiler will inline them.

And so, this the best thing to do in that it shows exactly what you care about in the code for the person reading, doesn't get bogged down with calls to big methods that do many other things that you don't care about in this case.

    if(str.isEmpty()){

Is the string empty? ... and we're done. The intent is clear and the code is simpler.

If, one was to have previously switched to a StringBuilder for this instead, there is no isEmpty() call, but there is a length() call that returns the count that should be used (source in AbstractStringBuilder 6-14).

---

What about the byte code for the StringBuilder.append vs += with a String? The += is much easier to read, what is the deal with that?

Lets take some quick code...

public void testMethod() {
    String foo = "";
    StringBuilder bar = new StringBuilder("");
    String qux = "";

    for(int i = 0; i < 100; i++) {
        foo += Integer.toString(i); // 38
        bar.append(i); // 39 
        qux = qux.concat(Integer.toString(i)); // 40
    }
}

and look at the byte code generated.

L6
 LINENUMBER 38 L6
 NEW java/lang/StringBuilder
 DUP
 INVOKESPECIAL java/lang/StringBuilder.<init> ()V
 ALOAD 1
 INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
 ILOAD 4
 INVOKESTATIC java/lang/Integer.toString (I)Ljava/lang/String;
 INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
 INVOKEVIRTUAL java/lang/StringBuilder.toString ()Ljava/lang/String;
 ASTORE 1
L7
 LINENUMBER 39 L7
 ALOAD 2
 ILOAD 4
 INVOKEVIRTUAL java/lang/StringBuilder.append (I)Ljava/lang/StringBuilder;
 POP
L8
 LINENUMBER 40 L8
 ALOAD 3
 ILOAD 4
 INVOKESTATIC java/lang/Integer.toString (I)Ljava/lang/String;
 INVOKEVIRTUAL java/lang/String.concat (Ljava/lang/String;)Ljava/lang/String;
 ASTORE 3

What does all of that mean?

Well, line 38 (the += line):

1. creates a new StringBuilder
2. invokes append on it with foo
3. invokes an Integer.toString to convert the integer to a String
4. invokes append on the string builder with the String from the previous step
5. converts the StringBuilder back into a String
6. stores that in foo

Note that the StringBuilder that was created can now be disposed of (along with the String that was created to allow the += to work because String += String is defined but not String += int.

On the other hand, line 39 does:

1. invoke StringBuilder.append with an int.

And as another option String.concat.

1. invokes an Integer.toString to convert the integer to a String
2. invoke String.concat (creates a new String object)
3. store that new string in qux

StringBuilder.append with an int is rather straight forward and optimized for putting an integer into an an array. There's a call to Integer.getChars in there that is the same as the code for Integer.toString.

The key to this is that there is no additional String being created (and discarded) for the Integer, nor a StringBuilder being created (and discarded) for the += when you use the StringBuilder in the first place.

Now, I'm moderately surprised (I knew it was the case, but still I'm moderately surprised) that the code for a single right hand side doesn't just do a String.concat(String) call which would probably be better (you've still got a bunch of Strings being created though its still less). However, the optimization would take additional work because if there was another += or a series of String foo = bar + qux + baz somewhere in that scope it would probably use the same StringBuilder as initially allocated.

Still, the point stands - if you are going to mutate a String, start off with a mutable string class and work with that. There are several ways to write more preformat code than += and given how Strings are used having a + inside a loop is setting yourself up for a disaster of unnecessary String objects floating around. String.concat can be good in certain cases when you are doing a single right hand side and not doing it repeatedly (the FizzBuzz code would be candidate for that), but a StringBuilder is still a better choice.

Answer 4

There's something wrong with each of them, but I think what's wrong with the first one is more acceptable.

The first has the defect that it requires unique prime factorization and the a posteriori fact that 3*5=15. That's a pretty well-known fact, but the problem was "if 3 then fizz, if 5 then buzz, if both then fizzbuzz." The problem says "if both." so write "both", which means "&&". You used a math trick to condense this expression when the original problem did no such thing, so you lost faithfulness to the original problem. Simplicity of the math trick is besides the point, in fact.

The second one is terrible, except I can think of one strength.

Character-by-character string manipulation induces character-by-character thinking. Why did it drag me down into this sort of analysis? It's far easier to type and think "FizzBuzz" than it is to ponder on how "FizzBuzz" is a beautiful thing, made up of its constituent parts.

Strings lend themselves to annoying off by one errors. I.e., where does the new line go now? It works correctly but I guarantee you it requires thought to write or evaluate.

The second program is more expandable. FizzBuzzBazz would be a much easier program to write given the work in problem 2. It's not quite data-driven, but it would very easy to accept an Array of Strings corresponding to prime factors 3,5,7,11... and turn the second program into a loop.

Also note that the problem statement in the more general case requires thinking about string concatenation. In general, I condone data-driven generalizations, so if the input to the program were a map of prime numbers to Strings, and it were implemented with a loop instead of an extra if block, I might call it clever but in the best of ways - it solves a more general problem, and data drives the magic numbers, and therefore has better separation of logic from appearance.

For the kicker, also note that the first is a step away from generalization.

So, it's applies to oranges: Invoking 3*5=15, or writing character-wise code that is halfway on the way to being more general. For what it's worth, I would prefer the first of the two.

Answer 5

Your technique has inferior extensibility/maintainability

Since no one mentioned extendability, I think it's worth comparing your coding techniques. Lets imagine that today the coding requirements were changed from the well known FizzBuzz to the new FizzBuzzCazz involving multiples of 7.

Your extended code:

public static void fizzbuzz(){
    for(int i=1;i<=100;i++){
        //Luckily we can skip i =3*5*7 = 105
        if(i%35 == 0) {
            System.out.println("BuzzCazz");
        } else if (i%21 == 0) {
            System.out.println("FizzCazz");
        } else if (i%15 == 0){
            System.out.println("FizzBuzz");
        } else if (i%7 == 0) {
            System.out.println("Cazz");
        } else if (i%5 == 0) {
            System.out.println("Buzz");
        } else if (i%3 == 0) {
            System.out.println("Fizz");
        } else {
            System.out.println(i);
        }
    }
}

The interviewer's extended code:

public static void fizzbuzz(){
    for(int i=1;i<=100;i++){
        String str = "";
        if(i%3 == 0){
            str = "Fizz";
        }
        if(i%5 == 0){
            str += "Buzz";
        }
        if(i%7 == 0){
            str += "Cazz";
        }
        if(str.equals("")){
            str = i;
        }
        System.out.println(str);
    }
}

See how much more extensible the interviewer's code is?

Answer 6

While the second implementation isn't terribly pretty, the first one makes me uneasy because it involves redundancy. Redundancy makes code harder to maintain and makes it more likely for bugs to creep in when you do have to maintain it.

Let's say for example that tomorrow, you were given a bunch of new requirements:

• Print "Feez" where you would have previously printed "Fizz"
• And do it for every factor of 8, not 3
• Print "Buzz" for every factor of 7 and every factor of 5
• In keeping with the original requirements, print "FeezBuzz" whenever both are applicable.

Requirements 1-3 each require modifying Example 2 in one place, while they require modifying Example 1 in two places each (six total, and #3 is particulary tricky to get right with Example 1).

Requirement 4 requires no changes to Example 2 at all, but requires making a modification to Example 1.

If you implement one of the new requirements in one place, but forget the other, your code is bugged. This is because Example 1 violates the DRY Principle, which is, in my opinion, one of the most important keys to writing quality code.

Answer 7

I like the first version better, because the structure of the code is more straightforward.

The second version may have a perceived advantage in having each of the strings "Fizz" and "Buzz" only once, but that comes at the cost of more complicated logic and an extra concatenation.

I think your version could be more readable by making the %3 and %5 checks explicit:

public static void fizzbuzz(){
    for(int i=1; i<=100; i++){
        if(i%3 == 0){
            if (i%5 == 0) {
                System.out.println("FizzBuzz");
            } else {
                System.out.println("Fizz");
            }
        } else {
            if (i%5 == 0) {
                System.out.println("Buzz");
            } else {
                System.out.println(i);
            }
        }
    }
}

It is a bit easier to see the connection between the rules as stated and the code, if the logic goes like this. This also eliminates the magical constant 15, which wasn't stated in the rules.

Answer 8

Both codes have their own advantages and disadvantages.

1. SPEED - your code is faster because it does not have overhead processing on concatenating Stringobjects. In Java, String is immutable. In effect, everytime a concatenation is performed, a new String object is created.

2. EXTENSIBILITY - your code is less extensible. I agree with @awashburn's answer. Your code is less flexible to cater to changing requirements.

3. MAINTAINABILITY - your code is more maintainable. You can trace the outputs directly because in every condition, you statically specified the target output.

4. RELIABILITY - your code is less reliable. Outputs are made up by combining words like Fizz, Buzz, etc. They are prone to typographical errors (human tendency).

5. REUSABILITY - both codes end up to be less reusable. The method should accept input and return output. Also, System.out.println should not be placed inside it.

Answer 9

Personally, I prefer your code.

The speed difference is nearly negligible; the bottleneck is IO. Hence, there's no need to go to the extent of rolfl's answer.

For the standard FizzBuzz problem, there is little need to be concerned about extensibility.

Most importantly, which piece of code can be more easily understood? Your code explicitly states what happens when a number is a multiple of 3, a multiple of 5, or both. On the other hand, it requires a bit of tracing to figure out the second piece of code. Hence, I'd prefer your code.

But if there's a possibility of the FizzBuzz problem being expanded to cover a huge list of numbers, neither solution can solve the problem in a clean way. A general FizzBuzz should be written instead.

Answer 10

To me, the second solution has a significant robustness problem. The first two if statements are fine (if you aren't concerned about performance). They perform a test on the input, produce a result, and assign it to an output variable, str. But the third test if (str.equals("")) is done on the output variable rather than on the input i. This is a convoluted way to perform the requested behavior, so the code is less readable. This methodology would be bug-prone in a more complicated problem.

In my opinion, the extensibility is worse than the % 15 in the first solution. Most comments saying the first solution is less extensible ignore the possibility that a requirement could change such that a factor of both 3 and 5 should print "Porcupine", or even that it should print absolutely nothing. In either case, your code would be easier to update.

I suggest that your answer would be ideal if you changed the first test to if (i%3 == 0 && i%5 == 0) (for clarity and extensibility despite the extra operation) and if you used constants to take advantage of the elegance of "FizzBuzz" happening to be a concatenation of Fizz and Buzz (which is the only "advantage" of the second solution):

private static final String OUTPUT_A = "Fizz";
private static final String OUTPUT_B = "Buzz";
private static final String OUTPUT_AB = OUTPUT_A + OUTPUT_B;