if and else or if and return?

Question

I have a Java method with a void type of return that checks a condition. It do something in case of true and other thing in case of false, typical if / else structure, but is possible use only the if block with a empty return in the end and omit the else block placing its contents with one less level of indentation. I don't know the implications of use one or other form, is one of they a bad practice? or use one or the other is a personal election?

// The first way
private void test(String str) {
 if (str.equals("xxx")) {
 // Do something
 } else {
 // Do other thing
 }
}
// The second way
private void test(String str) {
 if (str.equals("xxx")) {
 // Do something
 return;
 }
 // Do other thing
}

• amon: Possible duplicate of Should I return from a function early or use an if statement?
• Martin Spamer: It is related to that duplicate but not identical. I'd recommend using the approach that reads most naturally with the specific context it is used, rather than make a fixed rule.
• Erik Eidt: Either is fine, just don't use both together!!
• gnat: Possible duplicate of Clarification of "avoid if-else" advice
• Jules: The question is tagged "cyclomatic-complexity", but no issue about cyclomatic complexity is actually raised in the question. However, if this is something you're concerned about, note that cyclomatic complexity is calculated from the number of branching points in the code, and that doesn't change between if return and if else. Both of your example methods have a CC of 2.

Answer 1

It depends on the semantics of your code, if the else branch is the point of the method, i.e. it's named after what happens in the else branch, the early return is probably correct, especially if the first part is some sort of check.

If on the other hand the two branches are both related to the methods functionality, the if/else makes more sense to use.

ex.

void frob(...)
{
 if(isBlob())
 return; //can't frob a blob
 doFrobbing(...)
}

vs.

void condOp(...)
{
 if(condition)
 conditionalthing();
 else
 otherthing();
}

making the code match the semantics of your method makes it easier to read.

• Orici: I think, your example don't respond exactly to my question, the return in the first code example is a simple "safeguard clause", the if block only check if a condition is valid before to continue the execution of code, it don't execute nothing in it.
• Flater: @Orici: Imagine if the "safeguard clause" (as you call it) would also log a message that the safeguard clause was activated. While it is technically not just a return statement, it really still is a "safeguard clause". The question is where you draw the line of what is a safeguard clause and what is not. Finding a universal rule that is pedantically correct, in my opinion, is a futile effort. In most cases, the surrounding context of the method already makes it abundantly clear whether it's a "safeguard clause" or not.
• esoterik: @Orici I meant if your code resemebles the safeguard clause use that pattern, if it doesn't use what ever is cleaner
• candied_orange: @esoterik I really like making it about the name.
• Alexander Farber: Instead of if (str.equals("xxx")) it is safer to use if ("xxx".equals(str)). This might spare you an NPE when the str is null

Answer 2

If "something" and "other thing" are simple, straightforward, and understandable, it doesn't matter because both work.

// The first way is completely readable
private void test(String str) {
 if (str.equals("xxx")) {
 System.out.println("value is xxx");
 } else {
 System.out.println(str);
 }
 return;
}
// The second way is also readable
private void test(String str) {
 if (str.equals("xxx")) {
 System.out.println("value is xxx");
 return;
 }
 System.out.println(str);
 return;
}

However, if "something" or "other thing" are complex and hard to understand, it doesn't matter because both fail, and the solution is to refactor it into something simple enough that the code is back to the first case.

// The first way is completely readable
private void test(String str) {
 if (str.equals("xxx")) {
 doSomething(); // obfuscates the complex code you had before
 } else {
 doOtherThing(); // obfuscates the complex code you had before
 }
 return;
}
// The second way is also readable
private void test(String str) {
 if (str.equals("xxx")) {
 doSomething(); // obfuscates the complex code you had before
 return;
 }
 doOtherThing(); / /obfuscates the complex code you had before
 return;
}
/* 
 * Obviously, real code would have better variable and function names,
 * allowing the reader to understand what is happening.
 * If I care about how we doSomething(), I can investigate that function,
 * but I know that test(String) in some cases does something
 * and in other cases does the other thing, which may be enough
 */

• Orici: The last return is completely unuseful in a void method.
• Ecksters: Obfuscate generally has a negative connotation when it comes to code readability, I think "abstracts" or "pulls out" is the word you're wanting.