6
\$\begingroup\$

Problem:

Write a program that prints the numbers from 1 to 100. But for multiples of three print "Fizz" instead of the number and for the multiples of five print "Buzz". For numbers which are multiples of both three and five print "FizzBuzz".

Problem from here.

This is my code:

/*
 * Code by Clint
 */

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

Here's the output: Fizz Buzz output

\$\endgroup\$
  • 5
    \$\begingroup\$ I believe your post is unsuitable here, since your code doesn't do what the problem says to do.(e.g. for 15 it prints Fizz) \$\endgroup\$ – tinstaafl Jul 23 at 4:40
  • \$\begingroup\$ @tinstaafl It appears to be fixed with the last update. \$\endgroup\$ – Mast Jul 23 at 5:37
  • \$\begingroup\$ I realised that I haven't add the condition for the numbers that are divisible by 3 and 5. \$\endgroup\$ – user226918 Jul 23 at 5:39
  • 6
    \$\begingroup\$ Hello, according to the original exercise you should print FizzBuzz instead of Fizz whitespace Buzz for multiples of both three and five. \$\endgroup\$ – dariosicily Jul 23 at 6:47
  • 7
    \$\begingroup\$ Things divisible by 3 and 5 are divisble by 15. \$\endgroup\$ – slepic Jul 23 at 6:52
13
\$\begingroup\$

The code looks perfect, except for the variable name numbers. This variable only holds a single number, therefore its name must be number instead.

Since the variable is only used in a very small scope, the names n or i may be acceptable as well. (Subject to personal preference.)

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ The scope is literally the entire program. ;) \$\endgroup\$ – Fabian Röling Jul 24 at 11:01
  • \$\begingroup\$ By the way, if you want to write an enterprise-grade FizzBuzz, have a look at this project. \$\endgroup\$ – Roland Illig Jul 24 at 23:37
12
\$\begingroup\$

Imagine a variant on FizzBuzz with \$n\$ different say-this-syllable-if rules. Would you want to check \$2^n\$ cases? That will lead to either a lot of code (\$O(2^n)\$ lines) or deeply nested code (with \$O(n)\$ maximum indentation). Luckily, the problem statement does you a favour: on divisibility by 15, it asks you to print FizzBuzz, not Fizz Buzz with a space as you tried. So the clean-code approach (with \$O(n)\$ lines, and \$O(1)\$ maximum indentation) is more like this:

public class FizzBuzz {
    public static void main(String[] args) {
        for (int number = 1; number <= 100; number++) {
            String toPrint = "";
            if(number % 3 == 0) {
                toPrint += "Fizz";
            }
            if(number % 5 == 0) {
                toPrint += "Buzz";
            }
            if (toPrint.isEmpty()) {
                toPrint = Integer.toString(number);
            }
            System.out.println(toPrint);
        }
    }
}

Even in this case, where \$n=2\$, this approach has definite advantages. You easily avoid what-if-both problems, and it's obvious to a reader that you did. No need to think through boolean logic. Tom Scott discusses the advantages further here, albeit for JavaScript, not Java.

A separate issue is whether we even need all those braces.

(By the way, I depluralized your dummy variable's name, so it doesn't look like a list of numbers.)

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Although this answer does encourage to think further than the problem at hand, it might be too much for someone who's learning the basics. I used to always try to think afar, and was oftenly overengineering, trying to solve "potential future problems". Though I definitely agree with the answer. \$\endgroup\$ – Clockwork Jul 23 at 19:59
  • 1
    \$\begingroup\$ @Clockwork You have a point. Part of the reason I included that video link is because Scott gives such a good illustration of the gradualism in improving an algorithm. But yes, Fizz Buzz is usually intended as a "can you code at all?" test, rather than one of maintenance concerns in architecture (which is about all I could contribute that earlier answers hadn't discussed). Mind you, in interviews there can be follow-up questions. It's also worth noting the reasoning I gave is related to why we use guard clauses. \$\endgroup\$ – J.G. Jul 23 at 20:04
  • \$\begingroup\$ I thought this answer was about performance and yet we use String concatenation?? \$\endgroup\$ – whatamidoingwithmylife Jul 24 at 13:05
  • \$\begingroup\$ "I thought this answer was about performance" No, it's about concise, readable, maintainable code whose validity is easy to discern. FizzBuzz is never going to be a performance choke. \$\endgroup\$ – J.G. Jul 24 at 13:51
  • \$\begingroup\$ This solution depends on a possible coincidence that the output for a number satisfying multiple conditions is the concatenation of the outputs for each condition. Imagine a variation where the requirement for multiples of 15 were FB rather than FizzBuzz. OTOH it does demonstrate a good way to think about refactoring when the spec allows for it. \$\endgroup\$ – Barmar Jul 24 at 14:11
2
\$\begingroup\$

I suggest that you extract the evaluations in variables, to make the code a bit shorter and faster .

for (int number = 1; number <= 100; number++) {
   final boolean isFizz = number % 3 == 0;
   final boolean isBuzz = number % 5 == 0;

   if (isFizz && isBuzz) {
      System.out.println("Fizz Buzz");
   } else if (isFizz) {
      System.out.println("Fizz");
   } else if (isBuzz) {
      System.out.println("Buzz");
   } else {
      System.out.println(number);
   }
}

Edit

As specified in the comments, it's not worth of talking of speed in this case, due to the optimization done by the compiler and since I didn't do any verification.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Or even boolean methods \$\endgroup\$ – jmm Jul 23 at 18:47
  • 8
    \$\begingroup\$ re: "faster": Speculative micro-optimisations are bad. Always write the clearest code possible, until you have measured numbers that say you need to do otherwise. \$\endgroup\$ – Jonathan Hartley Jul 23 at 20:14
  • \$\begingroup\$ Related to @JonathanHartley's comment. \$\endgroup\$ – J.G. Jul 23 at 20:41
  • \$\begingroup\$ If your argument is about speed, then you should consider declaring those final variables outside the loop. Otherwise, the variables are reinitialized every time the loop runs. However, the compiler might optimize it for you and moreover, you need a couple of 100k iterations before you realize a difference. \$\endgroup\$ – momo Jul 24 at 7:15
  • \$\begingroup\$ @momo Any compiler that can't do that optimization (which you get for free if you use SSA anyhow) isn't worth worrying about. That said the same applies to the OPs original point. The much bigger problem is that the code is in main the main method so won't get JITed as a whole but at best via OSR which can result in much less optimal code. Also obviously it doesn't matter at all. \$\endgroup\$ – Voo Jul 24 at 10:26
1
\$\begingroup\$

Personally, i see multiple "Tasks" in that code

  • Start the application
  • Count from 1 to 100
  • Decide what to print for the current number
  • Print it

The "Start" is for me an extra Task, because if i want that my application behaves differently on different environments (count to 100 on my local machine, but count to 1 Million on my super computer), than this would be a place to get the desired information and then forward it to the "functionality".
Yes this is over engineered for this small code example, but we lean with small code examples and then apply it to big applications. Therefor i like to take the big gun for small examples, as long as the goal is training. :-)

In my eyes, the following code is much longer, but its easier to understand, because each methodname gives a "context" what will happen in it. That makes it easier to follow and understand the underlying code.

Also, when the "tasks" are logically separated, then its much easier to apply changes to it. Changing the "rules" would only mean to change the convertNumber function. Changing the way it should print the result would be only to change the output method. Also it would be quite easy to extract those functionalities in extra classes, and inject them. Then it would be a breeze to decide on the outside (environment) that the output should be done via System.out.println or via a graphical interface.

But as always, many ways bring us to our goal(s). And as always if you choose one way, then you get good things, but you have to pay for them. My approach gives flexibility, but it its much more writing. The minimal slower performance would be only an argument in a high performance environment, i think, where we have to count each cycle.

public class FizzBuzzApp {
    public static void main(String[] args) {
        FizzBuzz game = new FizzBuzz();
        game.playGame();
    }
}

public class FizzBuzz {
  public void playGame(){
    for (int numbers = 1; numbers <= 100; numbers++) {
      String result = convertNumber(number);
      output(result);
    }
  }
  
  private String convertNumber(int number) {
    if (numbers % 3 == 0 && numbers % 5 == 0) {
      return "Fizz Buzz";
    } else if (numbers % 3 == 0) {
      return "Fizz";
    } else if (numbers % 5 == 0) {
      return "Buzz";
    } else {
      return String.valueOf(number);
    }
  }

  private void output(String value) {
    System.out.println(value);
  }
}

Happy to hear about your opinion about my approach

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ You should review your code, since it does not compile. \$\endgroup\$ – Doi9t Jul 23 at 16:55
  • 3
    \$\begingroup\$ It reminds me of the Devil's Dictionary definition of structured programming: "A way to make programs longer, sometimes without degrading their performance". There is arguably some justification for making convertNumber a separate function, but the rest is just padding. (And "playGame" is a poor name for a function which has no user interaction at all - where is the "play" exactly?) \$\endgroup\$ – alephzero Jul 23 at 17:00
  • \$\begingroup\$ If this is the direction we are going, where is the dependency injection and interfaces? I kid. Sort of. I have found the simple Fizz Buzz challenge to be a good vehicle for introducing SOLID principles (or functional programming) to people because they can focus on the principles instead of the problem because it is so simple to understand. And yes, I have written a dependency-injected Fizz Buzz complete with injected sequence generator/iterator and "FizzBuzzer" classes \$\endgroup\$ – Colin Young Jul 23 at 19:02
  • \$\begingroup\$ Thanks Doi9t for the remark, Sorry, just wrote the code on the fly. \$\endgroup\$ – JanRecker Jul 24 at 5:37
  • 1
    \$\begingroup\$ There really is no reason to use a class here. Notice the missing state or virtual methods that would allow to adapt behavior. Just using static methods to separate concerns would get you the same advantages without all the boiler plate. If you want to use a class you should either make the output method abstract/virtual to allow different output. Or much better: Just pass a stream in the constructor where it should be written. But honestly you can do the same just in static methods without any downside. \$\endgroup\$ – Voo Jul 24 at 10:40
0
\$\begingroup\$

For me this formatting is a little easier to read, but basically the same:

public class FizzBuzz {
    public static void main(String[] args) {
        for (int i = 1; i <= 100; i++) {
            if (i % 3 == 0) System.out.println("Fizz");
            if (i % 5 == 0) System.out.println("Buzz");
            if (i % 3 != 0 && i % 5 != 0) System.out.println(i);
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ If someone said something like this to me in an interview, I would think they were trying to get a response rather than being honest. I would point them to "Making Wrong Code Look Wrong" and I would seriously reconsider whether I'd want to work there, if they really think that this is better code by a significant enough amount that it is worth "correcting". \$\endgroup\$ – Jasper Jul 23 at 15:39
  • 2
    \$\begingroup\$ In my opinion, not adding the brace in loop / if is way more error prone, since you can easily break the logic by forgetting to add them. \$\endgroup\$ – Doi9t Jul 23 at 17:08
  • 7
    \$\begingroup\$ This solution also has a bug. If i is divisible by both 3 and 5, it will print out Fizz\nBuzz (Fizz and Buzz on separate lines), not FizzBuzz. You could get around this by starting a newline either at the beginning or end of each iteration of the loop and using the print method instead of println \$\endgroup\$ – Seth R Jul 23 at 18:47
  • 1
    \$\begingroup\$ Ignoring the bug, the removal of {} goes against my very core. And I don't even use a language that uses {} for control flow. \$\endgroup\$ – Peilonrayz Jul 23 at 19:16
  • 1
    \$\begingroup\$ @Peilonrayz I too have added that to my on-save code-formatter. Too many times I've seen someone add a second line and not braces... \$\endgroup\$ – corsiKa Jul 24 at 0:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy