Efficient FizzBuzz

Question

I wrote the following Fizz Buzz program. How can I improve it or make it more efficient?

public class FizzBuzz 
  {  
   public static void main(String[] args)
   {
    System.out.println("--------Fizz Buzz program-------------");
    for(int num=1;num<=100;num++)
     { 
      if(num%3==0)
       {
        if(num%5==0)    
        System.out.println("FizzBuzz");
        else
        System.out.println("Fizz");
      }
    else if(num%5==0)
     {
        if(num%3==0) 
        System.out.println("FizzBuzz");
        else
        System.out.println("Buzz");
     }
     else
     System.out.println(num);
    }
   }
  }

Answer 1

The issues I see with your code are:

• Indentation: Your indentation is inconsistent. One space per level is insufficient for readability.

• Brace style: Leaving out the optional braces can lead to bugs when code is modified later. If you want to omit the braces, then also put the body on the same line to avoid any misconceptions.

  if (num % 3 == 0)
  {
      if (num % 5 == 0) System.out.println("FizzBuzz");
      else              System.out.println("Fizz");
  }
  

  Personally, I would advise that you stick with the standard Java conventions to make everyone's life easier. It also relieves the temptation to skimp on braces in the first place.

  if (num % 3 == 0) {
      if (num % 5 == 0) {
          System.out.println("FizzBuzz");
      } else {
          System.out.println("Fizz");
      }
  }
  

  By the way, opening braces on a separate line can be brutal when it comes to try-catch-finally blocks.

  try
  {
      …
  }
  catch (Exception e)
  {
      …
  }
  finally
  {
      …
  }
  

• Redundancy: The multiple-of-15 case is handled twice; it should need to be handled once.

• Repetition of System.out.println(): I suggest using a ternary conditional expression instead.

  for (int num = 1; num <= 100; num++) {
      System.out.println((num % 15 == 0) ? "FizzBuzz" :
                         (num %  3 == 0) ? "Fizz" :
                         (num %  5 == 0) ? "Buzz" :
                                           num);
  }
  

Answer 2

You have some redundant code there .... there is no need for the code :

   if(num%3==0) 
       System.out.println("FizzBuzz");
   else

in the else if(num%5==0) side of the program. This is because if the number was % 3 ==0 it would have gone to the first block.

Then in general, looking at the logic, I can't help but think there's a trick you are missing:

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

This won't be the fastest option, but it sure looks neat.

Answer 3

You can use just one modulus, but given the writing to the console is far, far more expensive than anything else changing the code without changing the output is unlikely to matter. i.e. removing one character from the output could make more of a difference.

for(int num = 1; num <= 100; num++) {
    switch(num % 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;
    }
}

Note: Java doesn't compile to native code until a loop or method has been called 10,000 times by default. If you warm up you code it can be 20x faster without even changing it. i.e. if you haven't warmed up your code, trying to make it more efficient doesn't really matter.

Answer 4

Most style related issues have already been addressed so I'll focus on design:

1. All your code is in the main method. This makes it not reusable. You should start refactoring it by creating something like a FizzBuzzGame class with a play method and move the code in there.

2. Next your upper limit is hard coded to 100. If you have followed the above step then add a parameter to the constructor which lets the user define the upper limit.

3. Your output is tied to standard out. What if you wanted to write it directly to a file, a network stream or just another programmer using the sequence?

   There are some options to remove this implicit dependency on standard out:

   1. The play method could accept an Appendable or a Writer and use it to write the result to it.
   2. Return the results as a list from the play method.

While this might possibly seem like a bit of overkill for this specific problem it's still a good idea to practice good design implementations.

It's a bit like muscle memory: If you just want to quickly hack together some code to test something and you tend to do it in a crappy way then chances are that you are prone to write more crappy code in production as well. However if you practice and attempt to write reasonably well designed code (at least in terms of dependencies and encapsulation) then it becomes easy and natural over time and your production code is more likely to be of a higher quality without you actually having to put a lot of conscious effort into it.

Answer 5

Here is an example of what probably ends up being a pointless optimization, but it is a completely different approach to the other posted solutions.

Generally integer addition and subtraction take fewer cpu cycles than multiplication and division. With that in mind you might try the following:

int fizzcnt = 0;
int buzzcnt = 0;
String msg;
for(int num = 1; num <= 100; num++) {
    fizzcnt++;
    buzzcnt++;
    if (fizzcnt == 3) {
       msg = "Fizz";
       fizzcnt = 0;
     }//this bracket was missing
    else {
       msg = "";
    }
    if (buzzcnt == 5) {
       msg = msg.concat("Buzz");
       buzzcnt = 0;
    }
    System.out.format("%s %d%n", msg, num);
}

Managed to write the whole thing with 1 loop, 2 if statements and no multiplication/division/modulus calculations. This should be more efficient. Maybe not. Notice the output message is created using String and concat. these little guys probably suck up a lot more cycles than were gained through elimination of the other statements. You would have to profile the code to really know.

The lesson here is it is probably better to work on readability and logical simplicity than it is to worry about efficiency - unless performance becomes a real concern. When efficiency does become a concern be prepared to spend a lot of time using a profiler and running benchmarks to verify that what you are doing is truly an improvement.