Java FizzString

Question

I am testing what knowledge I have with the FizzBuzz tests in Java. This is the one I just completed:

Given a string str, if the string starts with "f" return "Fizz". If the string ends with "b" return "Buzz". If both the "f" and "b" conditions are true, return "FizzBuzz". In all other cases, return the string unchanged.

My code (working):

    public String fizzString(String str){

    if (str.startsWith("f") && str.endsWith("b")) {
        return "FizzBuzz";
    }

    if (!str.startsWith("f")) {
        if (!str.endsWith("b")) {
            return str;
        } else return "Buzz";
    } else return "Fizz";
}

Questions: How can I make this code more efficient? Should I be assigning "Fizz" and "Buzz" to separate string variables, so that there is just one return statement at the end of the method? Should there be fewer if statements - if so, I'm not sure how it would be done. How would you go about the problem?

Answer 1

Here is a very direct translation of the problem into code.

public String fizzString(String str) {
    if (str.startsWith("f") && str.endsWith("b")) {
        return "FizzBuzz";
    } else if (str.startsWith("f")) {
        return "Fizz";
    } else if (str.startsWith("b")) {
        return "Buzz";
    } else {
        return str;
    }
}

Compared to your original code, it is easier to read because:

• There is no nesting
• All conditions are affirmative, not a mixture of affirmative and negative

There is no advantage to assigning the result to a variable and returning that variable.

If you want a single return statement, you can write:

public String fizzString(String str) {
    return str.startsWith("f") && str.endsWith("b") ? "FizzBuzz"
         : str.startsWith("f")                      ? "Fizz"
         :                        str.endsWith("b") ? "Buzz"
                                                    : str;
}

… which likely compiles to exactly the same code. Choose whichever version you think is more readable.

Answer 2

How can I make this code more efficient?

You don't really need to. It's a very simple method, I wouldn't expect any performance problems with it. If you really expect performance problems, you could save the result of startsWith and endsWith in a variable.

Should I be assigning "Fizz" and "Buzz" to separate string variables, so that there is just one return statement at the end of the method?

You can, but it's not necessary. The rule of only having one return doesn't have to be enforced, and often leads to less readable code.

Should there be fewer if statements

No, but they should be clearer. Your way of placing one if into the other, and of checking the negative case first makes the code hard to read.

If you turn the ifs around, your code would look like this:

// refactoring step one: negate ifs
if (str.startsWith("f") && str.endsWith("b")) {
    return "FizzBuzz";
}

if (str.startsWith("f")) {
    return "Fizz";        
} else {
    if (str.endsWith("b")) {
        return "Buzz";
    } else {
        return str;
    }
}

Note that I also changed your formatting (curly brackets for one-line statements and statement on its own line).

Now you can also notice that you have a if-else-if pattern, which would look a lot nicer as if-elseif:

// refactoring step two: use "else if"
if (str.startsWith("f") && str.endsWith("b")) {
    return "FizzBuzz";
}

if (str.startsWith("f")) {
    return "Fizz";        
} else if (str.endsWith("b")) {
    return "Buzz";
} else {
    return str;
}

Or, as you are returning, you could also write it as:

// refactoring step three (optional): just use if, and return
if (str.startsWith("f") && str.endsWith("b")) {
    return "FizzBuzz";
}

if (str.startsWith("f")) {
    return "Fizz";
}

if (str.endsWith("b")) {
    return "Buzz";
}
return str;

Answer 3

Hypothetically, if you really need a marginally higher-performance method call on the basis of number of steps involved...

public String fizzString(String str) {
    final boolean startsWithF = str.charAt(0) == 'f';
    final boolean endsWithB   = str.charAt(str.length() - 1) == 'b';
    if (startsWithF && endsWithB) {
        return "FizzBuzz";
    } else if (startsWithF) {
        return "Fizz";
    } else if (endsWithB) {
        return "Buzz";
    }
    return str;
}

The usual String methods are relatively fluent and I too think you should use them first. If you are really going to split hairs, then nothing is close to beating a charAt() call to access the underlying char[] array and then comparing the appropriate values. Store the start and end comparisons in two boolean fields, and use those values directly in the if clauses.

There's also one minor benefit of using the boolean results: You only need to change the comparison in one line (e.g. to str.charAt(0) == 'F') as opposed to two, which does help in its own little way to reduce the probability of introducing bugs in the future...