FizzBuzz using switches

Question

I'm learning about control structures now, and I want to see if I'm doing this as cleanly and efficiently as possible. Seems like there should be a less... verbose way of writing the switch but I wouldn't know what it is. Any criticism welcome!

FizzBuzz.java

public class FizzBuzz {

    public static final int fizz = 3;
    public static final int buzz = 5;

    public static void main(String[] args) {
        int status = -1;        

        for (int fbNumber = 1; fbNumber <= 100; fbNumber++) {

            if ((fbNumber % fizz == 0) && (fbNumber % buzz == 0)) {
                status = 1;
            } else if ((fbNumber % fizz == 0) && (fbNumber % buzz != 0)) {
                status = 2;
            } else if ((fbNumber % fizz != 0) && (fbNumber % buzz == 0)) {
                status = 3;
            } else {
                status = 4;
            }
            switch (status) {
                case 1:
                    System.out.println("FizzBuzz");
                    break;
                case 2:
                    System.out.println("Fizz");
                    break;
                case 3:
                    System.out.println("Buzz");
                    break;
                case 4:
                    System.out.println(fbNumber);
                    break;
                default:
                    System.out.println("Number could not be evaluated");
            }
        }
    }
}

Answer 1

Here's the thing. When you see

switch (status) {
    case 1:
    case 2:
    case 3:
    case 4:
    default:
}

what do you know? What does case 1 mean? Is 1 even a sensible status?

The same thing applies with

status = 1;
status = 2;
status = 3;
status = 4;

It's just not a meaningful thing to write.

There's also:

public static final int fizz = 3;
public static final int buzz = 5;

Although this kind of makes sense, it doesn't really make sense. Fizz isn't 3. 3 is just the factor that triggers "Fizz" in the output.

You should really be encapsulating the state not over the result (FizzBuzz/Fizz/Buzz/n) but over the input (Fiz/Not Fizz, Buzz/Not Buzz). Java doesn't have tuples, so let's use javafx.util.Pair<Boolean, Boolean>, and give it a loose abstraction:

public static class IsFizzOrBuzz extends Pair<Boolean, Boolean> {
    public IsFizzOrBuzz(boolean isFizz, boolean isBuzz) {
        super(isFizz, isBuzz);
    }

    public boolean isFizz() { return getKey(); };
    public boolean isBuzz() { return getValue(); };
}

Unfortunately you can't do case on objects, so the de-facto alternative is a hash map

static public Map<IsFizzOrBuzz, String> responses;
static {
    responses = new HashMap<IsFizzOrBuzz, String>();
    responses.put(new IsFizzOrBuzz(true,  true),  "FizzBuzz");
    responses.put(new IsFizzOrBuzz(true,  false), "Fizz");
    responses.put(new IsFizzOrBuzz(false, true),  "Buzz");
    responses = Collections.unmodifiableMap(responses);
}

Then the proper abstraction looks like

public static int FIZZ_FACTOR = 3;
public static int BUZZ_FACTOR = 5;

public static IsFizzOrBuzz classify(int n) {
    return new IsFizzOrBuzz(n % FIZZ_FACTOR == 0, n % BUZZ_FACTOR == 0);
}

public static String getResponse(int n, IsFizzOrBuzz classification) {
    if (responses.containsKey(classification)) {
        return responses.get(classification);
    }

    return String.valueOf(n);
}

public static void main(String[] args) {
    for (int i = 1; i <= 100; i++) {
        System.out.println(getResponse(i, classify(i)));
    }
}

I'm going to be honest, though. Although this works quite well in a concise language:

fizz_factor = 3
buzz_factor = 5

responses = {
    (True,  True):  "FizzBuzz",
    (True,  False): "Fizz",
    (False, True):  "Buzz"
}

def classify(n):
    return (n % fizz_factor == 0, n % buzz_factor == 0)

def get_response(n, classification):
    return responses.get(classification, str(n))

def main():
    for i in range(1, 101):
        print(get_response(i, classify(i)))

main()

in Java can you really call this worthwhile? You end up fighting the language most of the time. A simple

class FizzBuzz {
    public static int FIZZ_FACTOR = 3;
    public static int BUZZ_FACTOR = 5;

    public static String getFizzBuzzResponse(int n) {
        if ((n % FIZZ_FACTOR == 0) && (n % BUZZ_FACTOR == 0)) {
            return "FizzBuzz";
        }

        if (n % FIZZ_FACTOR == 0) {
            return "Fizz";
        }

        if (n % BUZZ_FACTOR == 0) {
            return "Buzz";
        }

        return String.valueOf(n);
    }

    public static void main(String[] args) {
        for (int i = 1; i <= 100; i++) {
            System.out.println(getFizzBuzzResponse(i));
        }
    }
}

is easy enough. YAGNI.

Answer 2

Constants fizz and buzz should be ALL_CAPS, by convention.

The use of status and the switch is unjustified. Why complicate things by setting a variable to alter behaviour a few lines later?

public class FizzBuzz {

    public static final int FIZZ = 3;
    public static final int BUZZ = 5;

    public static void main(String[] args) {
        for (int fbNumber = 1; fbNumber <= 100; fbNumber++) {

            if ((fbNumber % FIZZ == 0) && (fbNumber % BUZZ == 0)) {
                System.out.println("FizzBuzz");
            } else if ((fbNumber % FIZZ == 0) && (fbNumber % BUZZ != 0)) {
                System.out.println("Fizz");
            } else if ((fbNumber % FIZZ != 0) && (fbNumber % BUZZ == 0)) {
                System.out.println("Buzz");
            } else {
                System.out.println(fbNumber);
            }
        }
    }
}

Answer 3

Well, this immediately jumps out at me.

What does your status mean?

We should use some sort of enumeration or at least defined constants to give these statuses names. That will save a lot of maintenance time. Instead of having to read your code to determine what each scenario is, I can just read the variable name where it says: case kStatusFizzBuzz.

public enum FBStatus {
    FBStatusFizzBuzz,
    FBStatusFizz,
    FBStatusBuzz,
    FBStatusNone,
    FBStatusUnknown
}

And declare your variable as type FBStatus, initialized to FBStatusUnknown.

---

We shouldn't clutter the main function. We should be writing other methods and passing the work out to them.

For example, we might have one method which simply takes an integer, any integer, and returns the appropriate fizz buzz string.

And another method that serves as our test method, which takes a starting number, and a stopping number, and prints the result of the first method for each value.

Now all main has to do is call the testing method for the expected range.

For example:

public static String fizzBuzzString(int fbNumber) {
    // return appropriate string
}

public static void fizzBuzzTest(int start, int stop) {
    for (int i = start; i <= stop; ++i) {
        System.out.println(fizzBuzzString(i));
    }
}

public static void main(String[] args) {
    fizzBuzzTest(1,100);
}

---

It should also be noted that in your if-else chain, you're doing some unnecessary checking:

if ((fbNumber % fizz == 0) && (fbNumber % buzz == 0)) { // condition 1
    status = 1;
} else if ((fbNumber % fizz == 0) && (fbNumber % buzz != 0)) { // condition 2
    status = 2;
} else if ((fbNumber % fizz != 0) && (fbNumber % buzz == 0)) { // condition 3
    status = 3;
} else {  // condition 4
    status = 4;
}

Notice how in condition 4, we don't have to check for (fbNumber % fizz != 0) && (fbNumber % buzz != 0)? Why? It's because condition 4 only happens if nothing else happened.

So with that said, if we make it past condition 1, we know that fbNumber % fizz is non-zero or fbNumber % buzz is non-zero, but definitely not both. So in condition 2, if fbNumber % fizz == 0 evaluates to true, then we know the second half (fbNumber % buzz != 0) will always be true, because otherwise, we would've jumped into condition 1.

We can rewrite this if-else chain as such:

if ((fbNumber % fizz == 0) && (fbNumber % buzz == 0)) {
    status = 1;
} else if (fbNumber % fizz == 0) {
    status = 2;
} else if (fbNumber % buzz == 0) {
    status = 3;
} else {
    status = 4;
}

Answer 4

If we gradually eliminate some bad practices, or replace some techniques with better ones, then we can naturally arrive at something better. Let's go.

---

It's good to minimize the scope of variables as much as possible. Most notably in this example, the status variable is not used outside the body of the for loop, therefore it should be declared inside.

So instead of this:

public static void main(String[] args) {
    int status = -1;        

    for (int fbNumber = 1; fbNumber <= 100; fbNumber++) {

This is better:

public static void main(String[] args) {
    for (int fbNumber = 1; fbNumber <= 100; fbNumber++) {
        int status = -1;        

In this form, the variable status doesn't exist outside the for loop, which protects you from mistakenly reusing it where it was not intended.

This technique also reduces the live time of the variable. The live time is the number of lines between the first and last reference to a variable. The shorter the live time, the smaller the mental burden it takes to understand the variable, and the smaller the window where it might be misused.

---

Notice that the initialization status = -1 is unnecessary, because the if-then-else will always assign a value to it. So you can drop the pointless initialization, simply declare it without assignment:

int status;        

---

There are more problems with status:

1. You set it to values 1, 2, 3, 4, which by themselves don't carry any meaning. Looking at the numbers 1, 2, 3, 4, it's impossible to guess what they stand for, which is less than ideal in a program, and can easily lead to bugs.

2. After setting status in the if-else, you do a switch, which is a roughly equivalent operation, duplicating the effect of the earlier if-else. I think there's a term for this: status is a phony variable just for the sake of using in a switch, without a real functional purpose. (Notice the name "status" lacks a good functional meaning.)

3. The place in the code where you act on the different values of status is a couple of lines apart from the place where you set its value. This hurts readability: if I want to verify the connection between the action on a value and how that value was set, my eyes have to jump between the if-then-else block and the switch block.

In short, it would have been better to not use status at all: it caused more problems than it solved.

---

Applying the above changes we more or less arrive at the suggestion of @200_success, without a switch. This is not a good example case for using switch.

Answer 5

You could simplify this a little more by having a value for fizzbuzz:

public static final int fizzbuzz = fizz * buzz;

Thus, your first check will be simplified as such:

if (fbNumber % fizzbuzz == 0) {
    status = 1;

This may also be useful in case you decide to increase the number of different strings to be outputted. Instead of increasing the length of your first check, you'll just increase the value of fizzbuzz along with changing the variable name.

Answer 6

A switch is not warranted in this situation

Let's consider the problem: we have a collection of conditions. For each number, we want to test whether each condition matches the current number. For each matching condition, we output some magic value.

If there are no conditions that match, then output the original number.

The way to do this is to use an enum:

static enum FizzBuzz implements IntFunction<Boolean> {
    FIZZ(i -> i % 3 == 0, "Fizz"),
    BUZZ(i -> i % 5 == 0, "Buzz");

    private final IntFunction<Boolean> isApplicable;
    private final String output;

    FizzBuzz(final IntFunction<Boolean> isApplicable, final String output) {
        this.isApplicable = isApplicable;
        this.output = output;
    }

    public static String fizzBuzz(int i) {
        final String result = Stream.of(FizzBuzz.values())
                .filter(fb -> fb.apply(i))
                .map(FizzBuzz::getOutput)
                .collect(joining(""));
        if (result.length() > 0) {
            return result;
        }
        return Integer.toString(i);
    }

    @Override
    public Boolean apply(int value) {
        return isApplicable.apply(value);
    }

    public String getOutput() {
        return output;
    }
}

Each enum value has an IntFunction<Boolean> that defines whether it's applicable to the current number. The enum also has its magic String to output.

We define a method fizzBuzz that uses a Stream to:

• loop over the enum values in order
• check if the enum is applicable
• transform it to its magic String
• join all matching values on "" - i.e. just join them
• if the result is non-empty return the result, otherwise return the number

So use we just need to set up a Stream if numbers and process each with FizzBuzz.fizzBuzz and output the results:

IntStream.rangeClosed(1, 20)
        .mapToObj(FizzBuzz::fizzBuzz)
        .forEach(System.out::println);

This solution is very easily extensible to more conditions (divisors of 7 etc.). Your solution uses brute forced permutations rather than concatenation so using your logic you would have exponentially more tests for each new item.

A switch is used for a different situation, usually to replace chained if...else statements, not in addition to them.

Answer 7

(Very late to the party so just a very partial answer stating some general rules)

Why so complicated? My one-liner does the job as well:

System.out.print("1\n2\nFizz\n4\nBuzz\nFizz\n7\n8\nFizz\nBuzz\n11\nFizz\n13\n14\nFizzBuzz\n16\n17\nFizz\n19\nBuzz\nFizz\n22\n23\nFizz\nBuzz\n26\nFizz\n28\n29\nFizzBuzz\n31\n32\nFizz\n34\nBuzz\nFizz\n37\n38\nFizz\nBuzz\n41\nFizz\n43\n44\nFizzBuzz\n46\n47\nFizz\n49\nBuzz\nFizz\n52\n53\nFizz\nBuzz\n56\nFizz\n58\n59\nFizzBuzz\n61\n62\nFizz\n64\nBuzz\nFizz\n67\n68\nFizz\nBuzz\n71\nFizz\n73\n74\nFizzBuzz\n76\n77\nFizz\n79\nBuzz\nFizz\n82\n83\nFizz\nBuzz\n86\nFizz\n88\n89\nFizzBuzz\n91\n92\nFizz\n94\nBuzz\nFizz\n97\n98\nFizz\nBuzz\n");

Now seriously: Is yours any better? Not much, but it could be, just follow these rules (using other answers):

• Whenever you wrote a method doing both computation and output, you did it wrong as it's not reusable for anything else.

• Try to write methods returning a result to be used later (only when everything is done, print the results).

Make some nice building blocks like

FBStatus classify(int number) {...}

String fizzBuzzString(int number, FBStatus status) {
    switch (status) {
        case FIZZ: return "Fizz";
        case BUZZ: return "Buzz";
        case FIZZBUZZ: return "FizzBuzz";
        case NONE: return Integer.toString(number);
    }
}

compose them to bigger building blocks

String fizzBuzzString(int number) {
    return fizzBuzzString(number, classify(number));
}

and you get all the flexibility you may need later.

---

As already stated, using the switch means doubling the effort as you're first branching on the numbers and then on the status, which could with less effort done in one go.

In some cases, performance matters, in others, doing this makes it clearer without losing any significant speed. As fizzbuzz is too trivial, there's no clear answer here.

---

If I was to optimize, I'd go for something like

private static final String[] MOD15 = {
    "FizzBuzz", null, null,
    "Fizz", null, "Buzz",
    "Fizz", null, null,
    "Fizz", "Buzz", null,
    "Fizz", null, null,
};

String fizzBuzzString(int number) {
    String result = MOD15[number % 15];
    return result!=null ? result : Integer.toString(number);
}

but optimizations are rarely needed and that's a different story.

Answer 8

Few notes:

• Initialization of status is not required.
• Default status is 4, so the default case should be case 4.

• Precalculate and store the result instead of calculating at each step:

  int status = (((fbNumber%fizz==0)<<1) | ((fbNumber%buzz==0)<<1)) + 1;
  

• Consider using enum:

  public enum Status{
      FbStatusFizzBuzz("FizzBuzz"),
      FbStatusFizz("Fizz"),
      FBStatusBuzz("Buzz"),
      FBStatusDefault("Default")
      ;
  
      private final String status;
  
      Status(final String status) {
          this.status = status;
      }
  
      public String getStatus() {
          return this.status;
      }
  }