Code Review Asked by user226918 on December 6, 2021
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);
}
}
}
}
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.)
Answered by J.G. on December 6, 2021
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);
}
}
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.
Answered by Doi9t on December 6, 2021
Personally, i see multiple "Tasks" in that code
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
Answered by JanRecker on December 6, 2021
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);
}
}
}
Answered by Niklas Keck on December 6, 2021
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.)
Answered by Roland Illig on December 6, 2021
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP