10
\$\begingroup\$

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");
            }
        }
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ not enough factories! \$\endgroup\$ – xhainingx Dec 6 '14 at 23:08
  • 1
    \$\begingroup\$ So I'm a little annoyed that no one has yet suggested taking advantage of fallthrough, given that the question is about a "less verbose switch"... Don't have time to write up an answer now, though maybe later tonight if no one has since added one \$\endgroup\$ – Izkata Dec 7 '14 at 16:00
  • \$\begingroup\$ @Izkata, the fall-through behavior of switches makes code harder to read and understand, and should be avoided because it will increase the chance of bugs. It's better to factor out repetitive blocks of code into functions. \$\endgroup\$ – zzzzBov Dec 7 '14 at 16:44
  • \$\begingroup\$ @zzzzBov Yeah, I had to think about it a bit after making the comment (forgot to come back until now), but the particular feature I had in mind is from a different language (but would have been easier to read). Tiredness made me not realize it wouldn't transfer over to Java nicely. \$\endgroup\$ – Izkata Dec 22 '14 at 19:21
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Lots of great answers but yours feels the most thorough and useful for a beginner. Thanks for the great (now accepted) answer! \$\endgroup\$ – Phrancis Dec 9 '14 at 20:49
13
\$\begingroup\$

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);
            }
        }
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Your answer surely simplifies things, but it also goes to the wrong direction: You're mixing computation and output, which makes it completely non-reusable. Imagine you should process the output before writing it out. \$\endgroup\$ – maaartinus Dec 6 '14 at 15:59
  • 3
    \$\begingroup\$ @maaartinus There was actually no maintainability benefit to separating computation and presentation using a switch. You could, for example, call fizzbuzzWriter.writeFizz() to accomplish that without the awkwardness of setting a four-valued flag variable. And FizzbuzzWriter would actually be a modular, swappable part. \$\endgroup\$ – 200_success Dec 6 '14 at 16:08
  • 1
    \$\begingroup\$ I fully agree that switch makes little sense here. A method like fizzBuzzString plus a printing loop is perfect, IMHO. A java.io.Writer (if you mean it by FizzbuzzWriter) goes probably too far (especially for a beginner). \$\endgroup\$ – maaartinus Dec 6 '14 at 16:16
  • 1
    \$\begingroup\$ The if conditions can be simplified per nhgrif \$\endgroup\$ – jmoreno Dec 6 '14 at 17:44
  • \$\begingroup\$ @200_success Using an enum and returning that from its own method seems actually like a perfectly acceptable compromise between maintainability and complexity. \$\endgroup\$ – Voo Dec 7 '14 at 12:53
11
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ As a disclaimer, I basically never write Java... so take this all with a grain of salt. The core ideas here should be sound even if there's some Java-specific inaccuracies. \$\endgroup\$ – nhgrif Dec 5 '14 at 22:46
  • 2
    \$\begingroup\$ Contrary to the ole' horrible C enums that pollute the global namespace, Java's enums don't, so prefixing everything with the enum name just duplicates it. Also I see no reason to have an unknown state there in any case. \$\endgroup\$ – Voo Dec 7 '14 at 12:52
  • \$\begingroup\$ I'd prefix it for readability... But that may not be Java practice. \$\endgroup\$ – nhgrif Dec 7 '14 at 13:09
  • 1
    \$\begingroup\$ You are referring to an enum in java with its qualified name (except if you do static imports, but that's hardly a great idea in general). I don't see how FBStatus.FBStatusFizz is more readable than FBStatus.Fizz. \$\endgroup\$ – Voo Dec 7 '14 at 13:12
  • \$\begingroup\$ I thought when I saw an example of Java enums, the name wasn't qualified... But if it is, then I'd agree with your point. \$\endgroup\$ – nhgrif Dec 7 '14 at 13:15
7
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Isn't this solution a little overcomplicated? \$\endgroup\$ – Fabio Marcolini Dec 6 '14 at 16:18
  • \$\begingroup\$ @FabioMarcolini I was trying to demonstrate that switch is completely the wrong approach. This approach is complicated, yes, but for a reason. It also looks longer than it is - it's mostly Java boilerplate; once you remove the constructors and the getters... The OP's switch approach is complicated for no reason. \$\endgroup\$ – Boris the Spider Dec 6 '14 at 16:20
  • \$\begingroup\$ Sorry, but apart from the excessive length, I'd recommend it for Code Golf. While the OP's switch makes no sense, it may make it clearer in other situations. These functional stuff may, too, but only in very specific situations and otherwise it does a lot of harm. @FabioMarcolini Java 8 has become the unique way to write a few Java lines that nobody can understand. \$\endgroup\$ – maaartinus Dec 6 '14 at 17:17
2
\$\begingroup\$

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

\$\endgroup\$
  • 1
    \$\begingroup\$ FBStatus first thing that pops in my mind was Facebook status.(I'm a bit sad of myself :P) \$\endgroup\$ – Marc-Andre Dec 8 '14 at 16:20
2
\$\begingroup\$

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;
        }
    }
    
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.