18
\$\begingroup\$

I wrote this simple solution about FizzBuzz. Is there any possible way to solve or optimize the solution, such as with bitwise tricks?

public class FizzBuzz{
  public static void main(String[] args){

    for(int i = 1 ; i <= 100 ; ++i){
      if(i % (5*3) == 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);
    }
  }
}
\$\endgroup\$
  • 11
    \$\begingroup\$ Yes - see FizzBuzz Enterprise Edition \$\endgroup\$ – user11153 Dec 22 '14 at 12:46
  • 1
    \$\begingroup\$ I thought I'd remind you of the intention of FizzBuzz; it is an interview tool to asses basic programming proficiency; a step up from Hello World if you will. \$\endgroup\$ – AfterWorkGuinness Dec 22 '14 at 16:54
  • \$\begingroup\$ If you use System.out.print, and then a println at the end of each loop iteration, then your entire first condition -- (i % (5*3) == 0) { System.out.println("FizzBuzz"); } -- is no longer needed. \$\endgroup\$ – codingoutloud Dec 23 '14 at 1:05
  • 1
    \$\begingroup\$ Trying to optimize here is the wrong way to go on an interview, your #1 task is to get order of if statements correct, focusing on premature optimization might blur your concentration, and you can get the important things wrong. But even if you get the optimization right, it might not impress the interview, instead she might view you as someone prone to premature optimization. \$\endgroup\$ – Akavall Dec 23 '14 at 1:32
  • \$\begingroup\$ Note to reviewers: this being Code Review, your answer must have some bearing on the code in the question. If you advocate an alternative implementation, you must also justify why it is better than the original code. \$\endgroup\$ – 200_success Dec 26 '14 at 6:57

11 Answers 11

13
\$\begingroup\$

I like this.

if(i % (5*3) == 0){

Most people would have used 15 which I believe to be wrong. The way you've done it, it's clear that the first case is a multiple of the other two cases. Well done!

They are magic numbers though, and should be replaced with meaningful constants. Otherwise, I wouldn't change another thing. (Disclaimer: Someone will come behind me and talk about ways to make the code more efficient, but none of them will be as readable as this algorithm.)

\$\endgroup\$
  • 2
    \$\begingroup\$ Ehh. It's coincidental with these particular two numbers, but the same multiplication method doesn't work with, say, 6 and 8.. (No, they're not normally used on FizzBuzz, but multiplying like this is inferring things not in the typical FizzBuzz question) \$\endgroup\$ – Izkata Dec 22 '14 at 14:55
  • \$\begingroup\$ Yeah @Izkata I can kind of agree with that. I've taken the lowest common multiple route when I wrote on that was meant to be generic, but that's taking the concept to an abstract level it was never meant to be at. \$\endgroup\$ – RubberDuck Dec 22 '14 at 14:57
  • \$\begingroup\$ "They are magic numbers though" i disagree, in this context they make more sense than a named constant \$\endgroup\$ – user Dec 22 '14 at 15:41
  • 1
    \$\begingroup\$ You are missing what EVERYBODY misses. Notably, that "15" is NOT an additional condition. \$\endgroup\$ – Jasmine Dec 22 '14 at 20:35
  • 1
    \$\begingroup\$ Yes @VolodymyrMetlyakov. You're correct, it really should be the LCM. \$\endgroup\$ – RubberDuck May 3 '17 at 19:04
15
\$\begingroup\$

That's a classic FizzBuzz solution. I wouldn't try to do any clever optimization — it won't make any difference to performance. One hundred iterations of anything is trivial for a computer. Furthermore, most of the time will be dominated by the output routines, which you can't do much about.

The code formatting, on the other hand, could be improved. The most glaring issue is the omission of the braces for the final else. Why make your code ugly to save a couple of bytes?

Also note a de facto formatting convention: Put a space after if and for, and before any opening { brace.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. I'll read about this formatting guides. \$\endgroup\$ – Yodism Dec 22 '14 at 11:31
8
\$\begingroup\$

You have four calls to System.out.println(arg). If you want to change your code from printing to the console to writing to a file or a database, you have to change four lines in your code even though you're really only changing one "thing". Why not pull out the four System.out.println(arg)s and insert one after the if conditions like so:

public class FizzBuzz{
  public static void main(String[] args){

    for(int i = 1 ; i <= 100 ; ++i){
      String str;
      if(i % (5*3) == 0){
        str = "FizzBuzz";
      }else if(i % 3 == 0){
        str = "Fizz";
      }else if(i % 5 == 0){
        str = "Buzz";
      }else
        str = Integer.toString(i);

       System.out.println(str);
    }
  }
}

I agree with others that your if conditions are ugly.

\$\endgroup\$
  • 1
    \$\begingroup\$ the else without curly braces hurts my eyes. consistency is everything! \$\endgroup\$ – user Dec 22 '14 at 15:48
  • \$\begingroup\$ @user I left it in because I only wanted to make a point about the output calls. \$\endgroup\$ – user2023861 Dec 22 '14 at 16:36
  • 1
    \$\begingroup\$ that makes absolutely no sense at all :) \$\endgroup\$ – user Dec 23 '14 at 8:30
  • \$\begingroup\$ That makes no sense in the context of this program. You do not need to implement objects to hold information simply because you may use it more in the future. That would increase the size of all projects by a significant margin. \$\endgroup\$ – insidesin Aug 5 '15 at 22:06
6
\$\begingroup\$

From a general readability and maintainability standpoint this code could take some improvements.

The else if statements are really quite untidy and frequently end up being the source of trouble. A brace out of place there could cause a real mess - I'd look to refactor them out by putting the work into a function that converts int i into the string to output. It will make the code longer but also clearer because you have a separation of concerns where main controls the overall program flow and output, and the function converts the number to a string (replacing it with keywords as needed).

Once you've abstracted that work into a function you can also toy around with the many various ways that function could work without else if and test them independently of the main code. Some of those other implementations might be:-

  • early return (returning from the function as soon as you have the answer)
  • switch on the result of n mod 15 0=Fizzbuzz, [3,6,9,12]=fizz, [5,10]=buzz
  • string array lookup (based again on n mod 15)

Also, the 3 x 5 could stand some improvement; not because it should be 15 (because that is wrong too) but because they are 'magic numbers'. Neither 3 nor 5 should appear in the body of the code except as constants to define the values for the 'Fizz' and 'Buzz' states, that would mean 'Fizz x Buzz'.

Always consider that in the real world any code you make is likely to grow in complexity or length - getting those kinds of behaviours in place for a very simple piece of code make a huge difference when you start coding on a more complex project.

Aside: As a non-Java developer I was going to say that the K&R brace style is awful but on checking I found it masks an 'issue' in Java. Quite what whitespace is doing affecting output in a C style language I've no idea...

\$\endgroup\$
  • \$\begingroup\$ Interesting. Got a link for that @James? \$\endgroup\$ – RubberDuck Dec 22 '14 at 12:26
  • \$\begingroup\$ Which part, the braces bit? \$\endgroup\$ – James Snell Dec 22 '14 at 12:43
  • \$\begingroup\$ Yeah. I'm curious what issue it supposedly masks. (I'm not a Java guy either,) \$\endgroup\$ – RubberDuck Dec 22 '14 at 12:44
  • 1
    \$\begingroup\$ It's discussed on SO here: stackoverflow.com/questions/3218756/… - it says JavaScript there but I saw articles discussing the same in Java while I was looking. \$\endgroup\$ – James Snell Dec 22 '14 at 12:48
  • 1
    \$\begingroup\$ Very interesting to know, but that seems to apply to Javascript. This code is Java. Either way, thanks! \$\endgroup\$ – RubberDuck Dec 22 '14 at 12:51
6
\$\begingroup\$

Everybody noticed this, but no one actually said anything about it.

Your else statement is mismatched.

}else
    System.out.println(i);

This is not right in any language.

it should be

} else {
    System.out.println(i);
}

You have a multiple level if statement that makes use of bracing, the else statement must use bracing as well.

\$\endgroup\$
  • \$\begingroup\$ Ohh Sorry sir i did not noticed that. :) \$\endgroup\$ – Yodism Dec 22 '14 at 15:41
  • \$\begingroup\$ it's all good. it should still run, but it is not standard syntax, it could create bugs if you go to add something into the code later. \$\endgroup\$ – Malachi Dec 22 '14 at 15:43
  • \$\begingroup\$ "This is not right in any language." This isn't entirely accurate. It's correct, in the same way that "Buffalo buffalo Buffalo buffalo buffalo buffalo Buffalo buffalo" is correct. In both cases, though, if you say it in a room full of people who know about the subject, you'll get slapped silly. \$\endgroup\$ – Nic Hartley Jun 16 '15 at 2:53
  • \$\begingroup\$ Smart-alec, lol it works but the only circle it is accepted in is the code golf circle, not a good practice for production code. :) \$\endgroup\$ – Malachi Jun 16 '15 at 3:04
5
\$\begingroup\$

This chain if

if(i % (5*3) == 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);
}

is asking for maintenance trouble because you repeat 3 and 5 in unobvious way and distribute the arithmetic and comparison operations across the whole chain.

The following would be slightly better in regard to constant(s) use and also it first computes the actual conditions that matter and then reuses them here and there:

boolean isMultipleOf3 = ((i % 3) == 0);
boolean isMultipleOf5 = ((i % 5) == 0);
if(isMultipleOf3 && isMultipleOf5) {
    System.out.println("FizzBuzz");
} else if(isMultipleOf3) {
    System.out.println("Fizz");
} else if(isMultipleOf5) {
    System.out.println("Buzz");
} else {
    System.out.println(i);
}
\$\endgroup\$
3
\$\begingroup\$

I will reiterate the need for braces in your statements as well as the importance of avoiding magic numbers in your code. You also asked for how to 'solve' it and this is an alternative approach:

public class FizzBuzz {
    public static void main(String[] args) {
        final int fizz = 3;
        final int buzz = 5;

        StringBuilder sb = new StringBuilder("");
        for (int i = 1; i <= 100; i++) {
            if (i % fizz == 0) {
                sb.append("Fizz"); 
            }
            if (i % buzz == 0) {
                sb.append("Buzz");
            }
            if (sb.length() == 0) {
                System.out.println(i);
            } else {
                System.out.println(sb.toString());
                sb.setLength(0);
            }               
        }
    }
}

Why I like it: It's extensible, concise and flexible without giving up on readability. It remains immediately apparent how it works and is intuitive to add several more 'buzz' words and their checks.

I stopped preferring the standard method once It occurred to me how cumbersome additional conditionals would be to write in comparison.

Consider, for example, if you had to print Bazz for multiples of 7. With your code this would mean you would need to check for and print:

  • Bazz for multiples of 7.
  • BuzzBazz for multiples of 5 and 7.
  • FizzBazz for multiples of 3 and 7.
  • If you went beyond 100, FizzBuzzBazz for multiples of 3, 5 and 7.

This becomes exponentially more evident the more 'buzz' words you want to add and the higher the upper limit.

Meanwhile, this way, you would only need to add two already familiar lines:

final int Bazz = 7; to clearly designate that/eliminate ambiguity

and

if (i % bazz == 0) {
    sb.append("Bazz");
}

within the loop.

Here are the same ideas implemented using Java 8:

import java.util.stream.IntStream;

public class Java8FizzBuzz {
    static final int FIZZ = 3;
    static final int BUZZ = 5;
    static StringBuilder sb = new StringBuilder();

    public static void main(String[] args) {
        IntStream.rangeClosed(1, 100).forEach(Java8FizzBuzz::fizzBuzzify);
    }

    private static void fizzBuzzify(int i) {
        if (i % FIZZ == 0) {
            sb.append("Fizz");
        }
        if (i % BUZZ == 0) {
            sb.append("Buzz");
        }
        System.out.println(sb.length() > 0 ? sb.toString() : i);
        sb.setLength(0);
    }
}

Notice that both implementations have the added benefit of only needing to alter a single character, if for example rather than printing fizz for multiples of 3 one had to print fizz for multiples of 4. This is flexible FizzBuzz, however much that is worth.

\$\endgroup\$
  • \$\begingroup\$ As mentioned in another comment here, this does have the issue of still printing "Fizz" or "Buzz" in addition to a special string in case the client wants 15 to print "Hello World" instead of "FizzBuzz" :) \$\endgroup\$ – Juha Untinen Dec 23 '14 at 13:38
  • \$\begingroup\$ Although, that doesn't follow the pattern given, it's a testament to the real world thus a valid point. In that case this would become equivalently cumbersome to the traditional route. One case where something is the same and otherwise better than another method still means the something is superior in my book. \$\endgroup\$ – Legato Dec 23 '14 at 17:58
  • \$\begingroup\$ There is no need to use StringBuilder. Just use System.out.print where you are using sb.append and System.out.println at the end. \$\endgroup\$ – Simon Woodside Oct 13 '16 at 21:38
  • \$\begingroup\$ Means more overall print statements. Actually if I redesigned this now I would make it so there was a singular print statement after building the correct string to print. \$\endgroup\$ – Legato Oct 13 '16 at 21:40
3
\$\begingroup\$

It's hard to try and optimise the code after what's already mentioned. Here's a mathematical take on it:

  • Loop count = 100
  • %15 calculation = 100
  • %3 calculation = 100 - 6 = 94
  • %5 calculation = 100 - 33 = 67
  • Total modulu calculation = 261 (100 + 94 + 67)

Let's try this:

boolean isFizz = (i%3 == 0); // 100 times calculation
boolean isBuzz = (i%5 == 0); // 100 times calculation
// Total 200 times , a benefit of 61 for 100, 601 for 1000, etc i.e 23% :)
\$\endgroup\$
2
\$\begingroup\$

I would go for Java 8 with Streams and use a more functional approach:

IntStream.rangeClosed(1, 100).mapToObj(x -> "" + x).map(x -> {
    if (Integer.parseInt(x) % (5 * 3) == 0) return "Fizzbuz";
    if (Integer.parseInt(x) % 5 == 0) return "Buzz";
    if (Integer.parseInt(x) % 3 == 0) return "Fizz";
    return x;
}).forEach(System.out::println);

Simple and short. Whether it is better lies in the eye of the beholder.

\$\endgroup\$
  • \$\begingroup\$ +1 I personally appreciate the Java 8 take, and it's a good example of Lambda functions. \$\endgroup\$ – Legato Dec 26 '14 at 4:54
  • \$\begingroup\$ Nice. I appreciate your feedback. Speaking of Java 8 I never heard of it btw it's interesting to learn. \$\endgroup\$ – Yodism Dec 26 '14 at 5:47
  • \$\begingroup\$ @Patrick Oh? You should take a look into Java 8. It has some nice extensions (like strams and lambdas, default methods etc.) which look unusual at first sight, but make your life much easier. Although I wished Java would implement delegates, I am pleased with the new direction. \$\endgroup\$ – Thomas Junk Dec 26 '14 at 16:06
  • \$\begingroup\$ @Thomas Yes at first it looks unusual and hard to code. I'm currently reading about it. Thanks for mentioning Java 8. \$\endgroup\$ – Yodism Dec 27 '14 at 1:55
0
\$\begingroup\$

Is there any possible way to [...] optimize the solution, such as with bitwise tricks?

Bitwise tricks may optimize your code to ONE aspect (speed?). You have to say to which aspect you want your code to be optimzed. But that is not that easy as well. BTW I see a deleted answer that got downvoted as it provided a solution with bit shifting. The best comment to that was "We should write code for people to read".

But if you want to argue for readability: That is totally subjective if you have no real world anchor. If someone sees the code for FizzBuzz then he only can evaluate single statements and say what they do but he cannot evaluate an intention.

If you want to argue for extensibility optimization you have a hard day. FizzBuzz is nothing more than an exercise to train algorithmic thinking. There is no real world anchor so you have nearly no chance to forcast any abstractions that are neccessary for extensibility. Once you are thinking you have the correct abstraction someone can come up with some completely artificial requirement you haven't considered. I do not say that this does not happen with real world requirements but they tend to be easier to be forecasted.

Speed is indeed a measurement you can make. But can we really say that only the fastest code is the optimal code? I don't think so.

\$\endgroup\$
-3
\$\begingroup\$

To actually answer your question: "Is there any possible way to [...] optimize the solution? Like bitwise tricks?"

for (int i=1;i<=100;i++) {
    int a=((528>>i%15-1)&1)<<2;
    int b=((-2128340926>>(i%15)*2)&3)<<2;
    System.out.println("FizzBuzz".substring(a,b)+(a==b?i:""));
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Hi. Welcome to Code Review! We tend to prefer answers with some explanation as well as code. What's happening here? Why does it work? How did you establish that it is faster than the original method? \$\endgroup\$ – Brythan Dec 23 '14 at 10:40
  • \$\begingroup\$ How is that code works? Thanks :) \$\endgroup\$ – Yodism Dec 23 '14 at 10:47
  • 3
    \$\begingroup\$ -1 for two reasons. 1) You've just dumped code here. 2) We should write code for people to read, not machines. \$\endgroup\$ – RubberDuck Dec 23 '14 at 11:10
  • 3
    \$\begingroup\$ If you're going to use a bit pattern as a mini Turing machine, at least have the decency to write it as a binary literal, or at least hexadecimal. \$\endgroup\$ – 200_success Dec 26 '14 at 6:54

protected by 200_success Dec 26 '14 at 6:56

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

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.