Standard fizz (3) buzz (5)

Question

Is there a possibility to refactor this method and make it clean and respect solid principles.

public String pickOne(int input) {

        if ((input % 3) == 0 && (input % 5) == 0)
            return "fizzbuzz";
        else if ((input % 3) == 0)
            return "fizz";
        else if((input % 5) == 0)
            return "buzz";

        return String.valueOf(input);
    } 

Answer 1

If simplification

First if can be simplified to input % 15. Basically you are checking for number divisible by 3 and 5, so you can just check for 15. I agree with using brackets even here as mentioned by @Captain Man

public String pickOne(int input) {

    if (input % 15 == 0) {
        return "fizzbuzz";
    } else if (input % 3 == 0) {
        return "fizz";
    } else if(input % 5 == 0) {
        return "buzz";
    }

    return String.valueOf(input);
} 

Constructing the string from pieces

You can reduce one if by removing "else" conditions. Downside is, that you need to add another if in the end for the return values so probably not better, but should be mentioned:

public String pickOneV2(int input) {
    var sb = new StringBuilder();
    if (input % 3 == 0) {
        sb.append("fizz");
    }
    if (input % 5 == 0) {
        sb.append("buzz");
    }

    if (sb.length() == 0) {
        return String.valueOf(input); 
    }        
    return sb.toString();
}

Data-oriented approach

With all if conditions being in the same format, you can extract this into a data structure and simplify your code even more. I kept the map in the same method for simplicity reasons, but in reality it should be class property or a constant. This is the most extensible and clean I can think of, and you can easily modify this functionality purely by modifying the data without touching your code.

public String pickOneV3(int input) {
    var fizzbuzzMap = new TreeMap<Integer, String>(Comparator.reverseOrder());
    fizzbuzzMap.put(3, "fizz");
    fizzbuzzMap.put(5, "buzz");
    fizzbuzzMap.put(15, "fizzbuzz");

    for (var index : fizzbuzzMap.keySet()) {
        if (input % index == 0) {
            return fizzbuzzMap.get(index);
        }
    }

    return String.valueOf(input);
}

Using stream API

Slightly nicer version (imho) using streams:

public String pickOneV4(int input) {
    return fizzbuzzMap.entrySet().stream().filter(
            entry -> input % entry.getKey() == 0
    ).map(Map.Entry::getValue).findFirst().orElse(String.valueOf(input));
}

Answer 2

You have the most simple and minimalistic example of a fizzbuzz there can be. You could improve it by making the method static, since its state does not depend on any external field, and by following standard code formatting (if-elses should always have curly brackets). Maybe store the result in a variable and returning it in the end instead of having four different method exit points. And name the method something that is related to fizzbuzz instead of just "pick one". But that's all there is.

To make it "respect" SOLID principles you need to understand what SOLID means. All of the principles involve extensibility and modularity. Since there is literally nothing to extend or modularize in a fizzbuzz program, you would have to define how you want to break the fizzbuzz specification and then you could start talking about extending it. But in my opinion that's just forcing features on a program that does not need them. So do you want to make it output different words, use different values for the modulo operations, etc? Before you write the answer below, you need to write the code first and make it work and then you can ask for a SOLID review for what you have produced.

Out of curiosity... Where did you get this assignment? It's the second near identical question about fizzbuzz and SOLID principles in a short time. I find it a bit weird that it is that of a popular problem.

Answer 3

I suggest always using braces on any if statement. It's too easy to accidentally screw up something if you don't. It doesn't add many extra lines, especially with long chains like this since it only adds one extra line at the end.

public String pickOne(int input) {

        if ((input % 3) == 0 && (input % 5) == 0) {
            return "fizzbuzz";
        } else if ((input % 3) == 0) {
            return "fizz";
        } else if((input % 5) == 0) {
            return "buzz";
        }

        return String.valueOf(input);
    }

There's been major security issues for things like this:

if (user.isAdmin())
    log("Admin user logged in.");
    doImportantAdminThings();

Can you spot the flaw? Click below to see if you got it correct.

doImportantAdminThings(); is executed even when user.isAdmin() is false. The indentation makes it easy to mistakenly think that it's part of a block when it actually isn't. Now, in this small sample it's easy to spot, yes, but in massive code reviews it is easy to miss.