Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm a first year college student, Computer Science major. This is my crack at the FizzBuzz interview question in Java. What kind of improvements could I make?

//Prints numbers 1 - 100, 25 values per line.
//Numbers that are multiples of 3 are replaced with Fizz
//Numbers that are mulitples of 5 are replaced with Buzz
//Numbers that are multiples of both 3 and 5 are replaced with FizzBuzz

public class FizzBuzz {

    public static void main(String[] args) {

        String fizz = "Fizz";
        String buzz = "Buzz";
        String fizzBuzz = "FizzBuzz";

        //Loop from 1 through 100
        for (int i = 1; i <= 100; i++) {

            if (i % 3 == 0 && i % 5 == 0) {          //Checks for numbers that are both multiples of 5 and 3
                if (i % 25 != 0) {                   //Control flow allows 25 values printer per line
                    System.out.printf("%4.8s ", fizzBuzz);                  
                }
                else {
                    System.out.printf("%4.8s\n ", fizzBuzz); 
                    System.out.println();
                }

            }

            else if (i % 3 == 0) {                   //Checks for numbers that are multiples of 3
                if (i % 25 != 0) {                   //Control flow allows 25 values printer per line
                    System.out.printf("%4.8s ", fizz);
                }
                else {
                    System.out.printf("%4.8s\n ", fizz);
                    System.out.println();
                }

            }

            else if (i % 5 == 0) {                   //Checks for numbers that are multiples of 5
                if (i % 25 != 0) {                   //Control flow allows 25 values printer per line
                    System.out.printf("%4.8s ", buzz);
                }
                else {
                    System.out.printf("%4.8s\n ", buzz);
                    System.out.println();
                }

            }

            else {                                   //Prints numbers that are not multiples of 3 or 5
                if (i % 25 != 0) {                   //Control flow allows 25 values printer per line
                    System.out.print(i + " ");
                }
                else {
                    System.out.println(i + " ");
                    System.out.println();
                }
            }
        }
    }
}
share|improve this question
3  
If i divides 25 and we know that i fails to divide 5; what does this tell us about i? – Taemyr Feb 29 at 13:14
    
We ask this question in our interview process as a 'whiteboard' test. It's really just an acid test to see if someone can program at all vs someone that knows 'the right things to say'. Coming up with the most optimal solution is not required, although a truly hideous solution would raise eyebrows. However, if someone did come up with the "perfect solution" I would be very suspicious that they knew about the question and would ask for one more sample. You'd be surprised how many people over think/get too nervous and melt down on what is actually a fairly simple exercise. – Mike Feb 29 at 13:51
    
See my question about this that I had in an interview a while back. There are a lot of great answers on it: codereview.stackexchange.com/questions/60145/… – RDSpinz Feb 29 at 16:01
2  
This is meant tongue-in-cheek, but demonstrates the kind of software development you may do at a large firm: github.com/EnterpriseQualityCoding/FizzBuzzEnterpriseEdition – Jesse C. Slicer Feb 29 at 19:12
1  
I think this is a nice extension to FizzBuzz because it might cause a beginner to fall into the trap of coding 8 cases. Now add some more rules and you'll need to write 16, 32, 64 ... cases. That drives the point home. – usr Feb 29 at 19:28
up vote 27 down vote accepted
  1. System.out.printf("%4.8s ", ...); is actually not necessary here: this code means that you will format your String so that it has at minimum 4 characters, at most 8. Since "Fizz", "Buzz" and "FizzBuzz" have a length between 4 and 8, this will result in just outputing the String with no change. Therefore, it is simpler to have System.out.print(... + " ");.
  2. You have one part of your code that is repeated 4 times. Code duplication is something you want to avoid at all costs. It is better to refactor this duplicated logic into a small reusable method that you can then call elsewhere.
  3. In the same way, you can store inside two booleans the result of whether the number of divisible by 3 and 5.

In this case, you are repeating the check to print only 25 values per line, when you could do this only a single time after all the if/else logic.

You can then write your code a lot more simply:

for (int i = 1; i <= 100; i++) {
    boolean shouldFizz = i % 3 == 0;
    boolean shouldBuzz = i % 5 == 0;
    if (shouldFizz && shouldBuzz) { // Checks for numbers that are both multiples of 5 and 3
        System.out.print(fizzBuzz);
    } else if (shouldFizz) { // Checks for numbers that are multiples of 3
        System.out.print(fizz);
    } else if (shouldBuzz) { // Checks for numbers that are multiples of 5
        System.out.print(buzz);
    } else { // Prints numbers that are not multiples of 3 or 5
        System.out.print(i);
    }
    System.out.print(" ");
    if (i % 25 == 0) { // Control flow allows 25 values printer per line
        System.out.println();
        System.out.println();
    }
}
share|improve this answer
1  
Amusingly, now the values 3 and 5 are in the variable names, so if they ever change you'd still change them everywhere (I would hope). But we shouldn't care about this, it's only FizzBuzz. We don't need to worry about specs changing. Making it "more general" for the sake of it leads to madness. – RemcoGerlich Feb 29 at 9:59
    
I don't think having those two println calls at the end is good. Couldn't you simplify with a single print("%n%n");? – SirPython Feb 29 at 22:49
    
boolean shouldFizz = i % 3 == 0 and boolean shouldBuzz = i % 5 == 0 makes refactoring a little easier and the variable names quite a bit better. – nhgrif Mar 1 at 0:32
    
@nhgrif Thanks for your valuable comments! I hadn't thought of naming the variables like that. – Tunaki Mar 1 at 8:13

Two of the key sins in software engineering are scope creep and gold plating. Scope creep is a sin committed by clients and managers. Gold plating is a sin committed by programmers. This is a perfect example of the latter. The requirements for fizz-buzz are very simple. There is nothing in those requirements regarding printing 25 values per line. An implementation of fizz-buzz that prints one value per line is just fine.

While Java is well known for being a long-winded language, even in Java, an implementation of fizz-buzz that is 63 lines long (counting blank lines and comments) is far too long. As an interviewer, I would congratulate you on getting the answer right. However, in the post-interview meeting, I would most likely recommend that you not be hired.


Even in Java, the fizz-buzz problem should be very small (allowing a couple of violations of very standard coding practices):

// Solve the fizz-buzz problem: See
// http://imranontech.com/2007/01/24/using-fizzbuzz-to-find-developers-who-grok-coding/
public class FizzBuzz {
    public static void main(String[] args)
    {
        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);
    }
}

Even after incorporating those very standard coding practices (the statements that follow for, if, else, etc. should be on separate lines and should be enclosed by curly braces), fizz-buzz should be small, even in Java.

share|improve this answer
1  
In your quest seeking the simpler solution, you fail to notice the comment in the first line of the post: //Prints numbers 1 - 100, 25 values per line. Even not normally present, it seems like this was part of the interview question for the OP. – holroy Feb 29 at 18:48
1  
Regardless, the requirement can be addressed by the addition of just one extra line at the end of the loop, rather than checking in each branch of the ifs: if (0 == i % 25) System.out.println(); – Dewi Morgan Feb 29 at 19:06
    
@holroy -- That is gold plating. Printing 25 values per line, at 8 characters plus a space per value, is anti-human. We sighted humans are quite lousy after 80 characters per line. (A non-sighted coworker from years ago felt pity on those of us who are so visually constrained.) Note that the BDFL of python felt that the 80 character limit was out of date. 79 characters is the max, or at least so says he. – David Hammen Feb 29 at 19:53
    
Just to encourage the OP, while this would indeed be a major red flag for a senior position, it is not too bad for a graduate level position and my recommendation would depend a lot on the rest of the interview. For starters, I would point out that this is very long-winded and just ask the interviewee if they can make it less so. – romkyns Feb 29 at 22:52
    
This answer might address gold-plating, but leaves the developer prone to paying for the scope creep sins of his clients & managers. – nhgrif Mar 1 at 0:30

Disclaimer: Generalizing FizzBuzz might not be for the faint of heart, and as such David Hammen has valid points related to scope creep and gold plating. Other answers focus on possible simplifications and other aspects, but in this answer I would like to go down the path of generalizing the FizzBuzz algorithm allowing for other numbers and additional words.

There are two major improvement I would make to your code:

  • When recognizing that your print statements are very equal, extract these to a single print at end of method so that you don't repeat your self
  • Similar, when recognizing that 15 is the multiple of 3 and 5, consider changing to only calculate the modulo's a minimum number of times. That is change your code to allow for memorising for the case of "FizzBuzz"

Generic FizzBuzzRule class

To help us achieve this goal, here is some code for keeping lists of FizzBuzz rules:

class FizzBuzzRule {
    public int value;
    public String string;

    public FizzBuzzRule(int value, String string) {
        this.value = value;
        this.string = string;
    }

    // Define default set
    static public final ArrayList<FizzBuzzRule> BASIC_RULES = new ArrayList<>();
    static {
        BASIC_RULES.add(new FizzBuzzRule(3, "Fizz"));
        BASIC_RULES.add(new FizzBuzzRule(5, "Buzz"));  
    }

A fizzyfy method

This code also defines the basic set as a static so that we can access the basic set of rules using FizzBuzzRule.basicRules.

Further to simplify the generation of the list we'll implement a dedicated method to fizzyfy a number with a give rule set, which can look like this:

public static String fizzyfy(int i, ArrayList<FizzBuzzRule> rules) {

    StringBuilder fizzy = new StringBuilder();

    for (FizzBuzzRule rule : rules) {
        if (i % rule.value == 0) {
            fizzy.append(rule.string);
        }
    }

    if (fizzy.length() > 0) {
        return fizzy.toString();
    } else {
        return Integer.toString(i);
    }
} 

Notice how we now use a StringBuilder to concatenate the different part of the fizzyfied number. This make it a lot more generic, and in addition it only calculates any modulo operation just once for a given set. Instead of doing it for 3, 5 and 15, it only does it for 3 and 5 in the basic version. This makes even more sense if you add more rules like printing "Baz" if dividable by 7, and "Shoe" if divisable by 11... This would in your code make a lot of if statements.

In this basic extension I use the modulo operation, i.e. i % rule.value to calculate the remainder of the division i / rule.value. Due to use of a class this allows for the extension to add different rules for other numbers and/or words. An alternate ruleset (inspired by Wikipedia could be defined as:

ArrayList<FizzBuzzRule> extendedRuleset = new ArrayList<>();
extendedRuleset.append(new FizzBuzzRule(3, "Fizz");
extendedRuleset.append(new FizzBuzzRule(5, "Buzz");
extendedRuleset.append(new FizzBuzzRule(7, "Pop");
extendedRuleset.append(new FizzBuzzRule(11, "Whack");

Another extension, inspired by Twenty ways to FizzBuzz (in Javascript), could be to take it to the extreme and introduce predicates (as in #12) in to the generic rule class. However I'm going to leave that for another post sometime in the future.

Rebuild the main loop

I see the argument for using System.out.printf() as you can limit the output and get nice columns. However here also you should try to avoid repeating your self, and you should use a format which makes more sense related to the present output.

To avoid repeating yourself I would make the end of line a separate statement, and when combined with the other elements we can end with something like:

public static void main(String[] args) {

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

        System.out.printf("%-8.8s ", fizzyfy(i, FizzBuzzRule.BASIC_RULES));

        if (i % 4 == 0) {
            System.out.println();
        }
    }
}

Notice how this code limits the string to be of exactly 8 characters long, and it adds an extra space as column separator. I opted for only 4 numbers in each column, but change that at will.

Another advantage of this approach is that you know have separated the business logic and presentation logic. The business logic is split into the rules definitions of FizzBuzzRule and "fizzyfication" through fizzyfy(), and finally the presentation logic is neatly grouped within the main() method.

If you want to, you could of course opt for having the fizzyfy() within the FizzBuzzRule class, and choose other names here and there. But still the general principle of not repeating yourself, and still allowing for modifications and extending the ruleset or presentation is maintained.

To use the alternate ruleset, extendedRuleset, replace the second argument of fizzyfy() from FizzBuzzRule.BASIC_RULES to extendedRuleset (and add the creation of the ruleset in front of the for loop).

share|improve this answer
2  
This is not a good implementation. The rules engine is a standalone function and requires the rules to be an ordered list. The correct output of the rule processing is dependent on the order in which rules are added and that order being persisted. The logic of the rule ('% value == 0') is also encapsulated outside the rule itself, which limits flexibility. If your goal is just to explain to a first year student how an enterprise implementation might solve FizzBuzz, it would be better to clean it up and add more explanation of why you are proposing the enterprise structure and how it works. – Eric Feb 29 at 17:03
    
@Eric, I'm contemplating on doing a version with Predicate<T> but I'm a little short on time just now. The extension I've made using an ordered list is the most basic extension as it extends the usage of modulo operators, whilst replacing the modulo rule is an even higher grade extension. But I'll try to be even more descriptive when I get around to editing it. – holroy Feb 29 at 17:09
public class FizzBuzz {
    public static void main(String[] args) {
        for (int i = 1; i <= 100; i++) {
            if (i % 15 == 0) {
                System.out.print("FizzBuzz");
            } else if (i % 3 == 0) {
                System.out.print("Fizz");
            } else if (i % 5 == 0) {
                System.out.print("Buzz");
            } else {
                System.out.print(i);
            }


            if ( i % 25 == 0 ) { 
                System.out.println(); 
            } else {
                // Separate our results.
                System.out.print(" ");
            }
        }
    }
}

This is a very simple white board question as such, the examiners asking this will most likely not be looking for modularity, so there is no point in storing local variables that are being initialized to a predetermine value, unless we were going to be modifying them, based off of the original question and post we aren't actually modifying them.

So I removed the stored string variables. Most results will in the end do a check for i % 5 && I % 3 to check if a resulting answer should be FizzBuzz you can simplify this with getting the lowest possible number they can both modulate into... simply put that is 3 * 5.

Another possible solution is doing 3 if checks without any elses they are

if(i % 3 == 0) System.out.print("Fizz");
if(i % 5 == 0) System.out.print("Buzz");
if(i % 3 != 0 && i % 5 != 0) System.out.print(i);

The issue with this solution is every single iteration we are doing 3 modulations, and 3 possible checks. That are not required. using the if and else if sequence in this results with best case scenario of only doing 1 modulation to determine the answer, worst case 3. This saves us some time.

Below I am doing the if i % 25 == 0 check to determine if we need a new line if we dont I am adding a space to the print out so we can actually see each individual results.

I would say the original post isn't a bad solution for a first attempt. just something to keep in mind if you have something that you are doing multiple times that is more then 3 or 4 lines of code there should be a way to encapsulate that, I say that with regards to the:

if (i % 25 != 0) {                   //Control flow allows 25 value printer per line
    System.out.printf("%4.8s ", buzz);
}
else {
    System.out.printf("%4.8s\n ", buzz);
    System.out.println();
}
share|improve this answer
    
I have used the "no else blocks" method you noted at least once in the past but the way you have written it won't work. Your final condition if(!(i % 15 == 0)) will find things divisible by 3 and 5 as long as they are not divisible by 15. So like: Buzz10 11 Fizz12 13 14 FizzBuzz 16 17 Fizz18. You need something like if(i % 3 != 0 && i % 5 != 0) for that final condition. – Mark Balhoff Feb 29 at 20:25
    
Mark, that is correct I will update it now – James Bean Mar 1 at 0:17

You basically repeat the same 7 lines 4 times. And what's worse is that, as written, half the time the full block isn't possible.

if (i % 25 != 0) {
    System.out.printf("%4.8s ", fizzBuzz);                  
}
else {
     System.out.printf("%4.8s\n ", fizzBuzz); 
     System.out.println();
 }

This is totally unnecessary on the %3 and number branch as those can never be executed when %25. It is also unnecessarily long.

System.out.printf("%4.8s ", fizzBuzz);   
if (i % 25 == 0) {
     System.out.println();
 }

Now the 7 lines are 4 lines, and it should also now be obvious that a further reduction can be made -- just do the final 3 lines once, outside of the conditions that determine what to print. Whether you print a newline or not, is unconnected to what you print, there's no reason to combine them.

Fixing the above moves it out of the "works but horrible implementation" category.

But as a general rule, I dislike mixing output and logic, and to a lesser extent loops and logic.

If you extract the logic in the loop out into it's own function, that returns a string, you make it easier to read, maintain and test.

share|improve this answer

Most of these answers, as well as the original proposed in the question, seem overly complex. A program producing the correct output should be about ten lines:

    public class fizzbuzz {

        public static void main(String[] args) {

            for (int num = 1; num < 101; num++) {

                if ( num % 3 == 0 ) { System.out.printf("Fizz"); }
                if ( num % 5 == 0 ) { System.out.printf("Buzz"); }
                if ( num % 3 != 0 && num % 5 != 0 ) { System.out.printf("%d ", num); }
                if ( num % 3 == 0 || num % 5 == 0 ) { System.out.printf(" "); }
                if ( num % 25 == 0 ) { System.out.println(); }

            }
        }
    }

The code presented simply loops through the range of 1-100. I use '< 101' rather than '<= 100' to simplify the loop comparison. The five IF statements simply check the current number, and print whatever is appropriate. Other solutions may use more complex nested IF statements, but my experience, and that of other programmers is that nested loops are often harder to follow and lead to more logic errors. If they can be avoided, they generally should. Other than that the program should be fairly self explanatory.

share|improve this answer
    
Um, no. This does not print 1, 2, 4, 7, 8, 11, 13, 14, 16, 17, 19, 22, 23, 26, ... – David Hammen Feb 29 at 16:37
    
You are correct. I've seen other fizzbuzz that only printed the words and skipped non matching numbers. I added that line. – Byron Jones Feb 29 at 16:40
2  
I cleared the downvote. However, repeating the same test multiple times to avoid nested if statements is not necessarily a good thing. – David Hammen Feb 29 at 17:06
1  
I was writing primarily about the need to print 25 values per line. That is something that is not in the fizz buzz problem statement. There is no need for that. Your program has an extended cyclomatic complexity of nine. (Remove the test for 25 values per line and the complexity drops by one.) The complexity can be dropped even further with an else or two. Nested if-then-else statements are not necessarily evil. Artificial complexity is, whether it's because of gold plating or fear of intimidating people who should never be programming in the first place. – David Hammen Feb 29 at 17:25
1  
The original code states the following in the first line: //Prints numbers 1 - 100, 25 values per line. – Byron Jones Feb 29 at 18:23

You could do two (2) small loops to do this; first one will fill out an array and the second one will print 25 array cells per line.

public static void main(String args[]) {

    String[] storage = new String[101];

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

        if (i % 3 == 0 && i % 5 == 0) {
            storage[i] = "FizzBuzz";
        }
        else if (i % 3 == 0) {
            storage[i] = "Fizz";
        }
        else if (i % 5 == 0) {
            storage[i] = "Buzz"; 
        }
        else {
            storage[i] = Integer.toString(i);
        }

    }

    // Print data
    for(int i = 1; i < storage.length; i++) {
        System.out.print(storage[i] + " ");
        if(i % 25 == 0) {
            System.out.print("\n");
        }
    }
}

This is a little bit more code than required for the simplest and most straight forward solution, but with an array, the data would be reusable. Maybe you want to print it backwards? It's silly to do the same processing over and over again.

But in-case you absolutely know you will not use the data again, you can just print it out with maximum of 5 comparison operations per iteration:

public static void main(String args[]) {
    for(int i = 1; i <= 100; i++) {
        if (i % 3 == 0 && i % 5 == 0) {
            System.out.print("FizzBuzz ");
        }
        else if (i % 3 == 0) {
            System.out.print("Fizz ");
        }
        else if (i % 5 == 0) {
            System.out.print("Buzz "); 
        }
        else {
            System.out.print(i + " ");
        }
        if(i % 25 == 0) {
            System.out.print("\n");
        }
    }
}
share|improve this answer
    
This wastes both time and memory. Firstly you do not need to store the strings when you can just print them outright, secondly one of your array items isn't just just for the sake of omitting some additions or an extra variable, and lastly you are looping through the first loop 100 times and the second 101 times when you could just have a single loop iterating 100 times. If you are suggesting generating everything as a persistant list, do it properly with a generator function or encapsulate the logic into a function that outputs a string based on the number. – Pharap Mar 1 at 5:54
public class FizzBuzz{ 
  public final int FIZZ = 3;
  public final int BUZZ = 5;

  private String getFizzyBuzzyString(int num) {
    String fizzybuzzyString = "";
    fizzyBuzzyString += (num % FIZZ == 0)? "Fizz" : "";
    fizzyBuzzyString += (num % BUZZ == 0)? "Buzz" : "";

    return StringUtils.hasLength(fizzyBuzzyString)? fizzyBuzzyString : String.valueOf(num);
  }

   public static void main(String [] args){
     for (int i = 1; i < 101; i++){
       System.print(getFizzyBuzzyString(i) + " ");
       if (i % 25 == 0) {
         System.print("\n"); 
       }
    }
 }

Improvement

This moves the fizzbuzz logic away from the main method; a good thing to do for code interviews because it shows that you understand Separation of Concerns Your fizzbuzz method now only tells what the integer should be printed as), and the printing itself is handled by the main method.

Sidenote (irrelevant for most code interviews)

There is a con to this approach. This will be very slightly slower than simply having 4 if-clauses; the former checks divisibility by 3 first and then by 5 while the latter approach simply checks for 15 (see shortcuting in if statements). However, due to the fact that most numbers are not divisible by 15, the speed difference is miniscule in terms of complexity.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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