10

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
}
Iulian Onofrei
  • 303
  • 1
  • 12
Orici
  • 217
  • 1
  • 2
  • 6
  • 6
    Possible duplicate of [Should I return from a function early or use an if statement?](https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement) – amon Jul 16 '18 at 22:24
  • 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. – Martin Spamer Jul 16 '18 at 23:23
  • Either is fine, just don't use both together!! – Erik Eidt Jul 17 '18 at 01:29
  • Possible duplicate of [Clarification of "avoid if-else" advice](https://softwareengineering.stackexchange.com/questions/206816/clarification-of-avoid-if-else-advice) – gnat Jul 17 '18 at 01:40
  • 2
    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. – Jules Jul 17 '18 at 02:06
  • If we're being pedantic, returning in the middle of a method should be avoided entirely, if for no other reason that it makes it easier to read in terms of what is being returned by the method (this is a trivial example, but complex methods are increasingly difficult to read with multiple exit points). That said, I still do it if it isn't nested in another loop or if block as a means to validate input. – Neil Jul 17 '18 at 06:39
  • I revised the two possible duplicate entries, I think they don't ask exactly the same question. https://softwareengineering.stackexchange.com/questions/18454 show a safeguard clause, only check a condition to stop or continue with the rest of code in the method. The https://softwareengineering.stackexchange.com/questions/206816 ask about nested if-else blocks inside other if-else blocks – Orici Jul 17 '18 at 07:21

2 Answers2

18

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.

esoterik
  • 3,879
  • 3
  • 13
  • 22
  • 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. – Orici Jul 17 '18 at 07:16
  • @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. – Flater Jul 17 '18 at 12:37
  • @Orici I meant if your code resemebles the safeguard clause use that pattern, if it doesn't use what ever is cleaner – esoterik Jul 17 '18 at 17:53
  • @esoterik I really like making it about the name. – candied_orange Jul 20 '18 at 03:17
  • 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 – Alexander Farber Jan 21 '20 at 09:18
3

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
 */
Chris G
  • 204
  • 1
  • 5
  • 6
    The last `return` is completely unuseful in a void method. – Orici Jul 17 '18 at 07:13
  • 3
    Obfuscate generally has a negative connotation when it comes to code readability, I think "abstracts" or "pulls out" is the word you're wanting. – Ecksters Dec 18 '20 at 21:23