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

share|improve this question
11  
He may have been lying, but there's not a wrong answer when asked that question. There are people on both sides of the debate. I have a feeling he was trying to get a look at your thought process. –  RubberDuck Aug 15 at 16:00
7  
Interviewers doing deep analysis of FizzBuzz are kinda missing the point of FizzBuzz - to filter out the candiates who can't write anything. –  Justin Aug 16 at 23:19
1  
@Justin possibly the question is to filter out candidates who have memorized one correct FizzBuzz implementation –  emory Aug 17 at 21:06

9 Answers 9

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.

share|improve this answer
3  
I also think the string concatenation should be avoided, but that seems to be an endless debate. Either way, you have an Interesting implementation here. –  RubberDuck Aug 15 at 15:59
9  
Very interesting solution and presumably more performant, but bags more difficult to read and quickly understand in my opinion –  user3791372 Aug 15 at 20:53
    
That is pretty cool. I finally understand the point of requiring the break keyword at the end of most cases –  tel Aug 17 at 18:23

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)

share|improve this answer
6  
The reviewers solution is only more flexible when you assume "fizzbuzz" is the combination of "fizz" and "buzz". what if you have to print "fozzbozz" on every multiple of 15? –  user1895420 Aug 15 at 16:48
4  
@user1895420 That is true, but so far I have not seen such an assignment. I think it's more likely that the assignment is to add a "Woof" for another number. –  Simon André Forsberg Aug 15 at 16:54
2  
"It does not compile as str = i; is an illegal assignment" and the lack of stringbuilder are the biggest wtf's for me! –  user3791372 Aug 15 at 20:52
3  
Doesn't compile is something that I'd have rubbed in the interviewers face, regardless of context. "It's nice that you think it can be done differently, but this code works. Changing it without reason would be a bad idea... I mean, your code doesn't even compile, why would you change it like that?" and then flip the questions. –  Pimgd Aug 15 at 23:52
2  
I believe the String vs StringBuilder point is actually irrelevant. The compiler will convert the String concatenation to use a StringBuilder. It's only when your variable is declared outside the loop that it can't substitute in the StringBuilder, which results in extra overhead. Ie: this SO question/answer. –  Roddy of the Frozen Peas Aug 17 at 5:33

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

share|improve this answer
    
Urgh. Relying on string interning for presumed performance benefits without actually measuring? I'd say that's by far the worst thing you could ever do. The first thing that equals does is an identity comparison, so the performance benefit comes down to a possibly slightly reduced code size (I challenge anyone to measure the difference). isEmpty is the cleanest way to write this. –  Voo Aug 15 at 18:17
    
@Voo its not a performance thing, and it would be wrong for a number of reasons... and yep, already aware of it - "And its going to do that test anyways in the first test of .equals(Object anObject) in the String class." -- but its not the performance of the == when its the same thats at issue, its the additional work that it needs to do to set up that they are different when there are other ways that would work much more efficiently and convey the intent just as well. –  MichaelT Aug 15 at 18:20
1  
Fair point, but that's still only comparing the length of both internal arrays - even for the little work done I doubt that's measurable. But in any case going with isEmpty should be about as efficient as the identity comparison and wouldn`t rely on string interning. –  Voo Aug 15 at 18:24
    
Because the String is immutable, length is a private final int. The length of the methods isEmpty and length are both such that HotSpot will inline them quickly. The computation for the value is done in the constructor for the String and you've got an int vs int comparison compared to a reference vs reference comparison. Note that this is also in equals after a bit of type checking line 1019, but if you can make the test clear with isEmpty, its better. –  MichaelT Aug 15 at 18:30
    
re. StringBuilder, firstly are you sure += isn't optimized to avoid the realloc? Secondly isn't that overkill? This just somehow sounds like overkill to me. –  AAA Aug 16 at 18:12

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.

share|improve this answer
1  
"Strings lend themselves to annoying off by one errors. I.e., where does the new line go now?" - What are you talking about?? –  Simon André Forsberg Aug 15 at 16:43
1  
@SimonAndréForsberg too much experience as a C programmer. But there are intuitively equivalent, but incorrect ways to build the string. e.g. outputting early, if(i%3 ==0) println("Fizz"); if(i%5==0) println("Buzz") is bugged. Program 2 has an "output once" rule which deals with this squarely. –  AAA Aug 15 at 16:55
    
I would say that both versions are only doing output once, as the first one has a clear separation using a if-elseif chain. The new line problem is not very relevant when it's not a problem to begin with. –  Simon André Forsberg Aug 15 at 17:11
    
@SimonAndréForsberg I'm trying to make the point that no string manipulation is better than string manipulation. In the equivalent C program this problem would become horrendous in the second version only. –  AAA Aug 15 at 18:07
    
+1 for '3*5=15. That's a pretty well known fact'. Even quantum computers know this ia.ucsb.edu/pa/display.aspx?pkey=2803 –  Keith Aug 18 at 3:45

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?

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

protected by Community Oct 3 at 8:07

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.