5
\$\begingroup\$

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?

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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;
\$\endgroup\$
  • 2
    \$\begingroup\$ Your suggestion to do a if/else if/else is not correct. The code will never produce the string fizzbuzz, as specified in the problem statement \$\endgroup\$ – mleyfman Apr 1 '15 at 16:19
  • \$\begingroup\$ @mleyfman I just didn't include the if (str.startsWith("f") && str.endsWith("b")) check, because that stayed the same. But I can see how it's confusing that I did include it in the last code snipped. \$\endgroup\$ – tim Apr 1 '15 at 16:28
0
\$\begingroup\$

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

\$\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.