HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMajor

First solution to FizzBuzz

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
solutionfirstfizzbuzz

Problem

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 {

Solution


  • 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(... + " ");.



  • 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.



  • 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();
    }
}

Code Snippets

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();
    }
}

Context

StackExchange Code Review Q#121387, answer score: 27

Revisions (0)

No revisions yet.