18

I sometimes stumble upon code similar to the following example (what this function does exactly is out of the scope of this question):

function doSomething(value) {
  if (check1(value)) {
    return -1;
  }
  else if (check2(value)) {
    return value;
  }
  else {
    return false;
  }
}

As you can see, if, else if and else statements are used in conjunction with the return statement. This seems fairly intuitive to a casual observer, but I think it would be more elegant (from the perspective of a software developer) to drop the else-s and simplify the code like this:

function doSomething(value) {
  if (check1(value)) {
    return -1;
  }
  if (check2(value)) {
    return value;
  }
  return false;
}

This makes sense, as everything that follows a return statement (in the same scope) will never be executed, making the code above semantically equal to the first example.

Which of the above fits the good coding practices more? Are there any drawbacks to either method with regard to code readability?

Edit: A duplicate suggestion has been made with this question provided as reference. I believe my question touches on a different topic, as I am not asking about avoiding duplicate statements as presented in the other question. Both questions seek to reduce repetitions, albeit in slightly different ways.

  • 14
    The else is a lesser issue, the bigger problem is you are obviously returning two data types out of a single function, which makes the API of the function unpredictable. – Andy Dec 10 '16 at 17:55
  • 3
    E_CLEARLY_NOTADUPE – kojiro Dec 10 '16 at 19:00
  • 3
    @DavidPacker: At least two; could be three. -1 is a number, false is a boolean, and value isn't specified here so it could be any type of object. – Yay295 Dec 10 '16 at 21:53
  • 1
    @Yay295 I was hoping the value is actually an integer. If it can be anything then that's even worse. – Andy Dec 10 '16 at 22:42
  • @DavidPacker This is a very important point, especially when raised with regard to a question about good practices. I agree with you and Yay295, however the code samples serve as an example to demonstrate an unrelated technique of coding. Nevertheless, I keep in mind that this is an important matter and shouldn't be overlooked in actual code. – rhino Dec 12 '16 at 20:21
23

I like the one without else and here's why:

function doSomething(value) {
  //if (check1(value)) {
  //  return -1;
  //}
  if (check2(value)) {
    return value;
  }
  return false;
}

Because doing that didn't break anything it wasn't supposed to.

Hate interdependence in all it's forms (including naming a function check2()). Isolate all that can be isolated. Sometimes you need else but I don't see that here.

  • I generally prefer this also, for the reason stated here. However, I usually place a comment at the close of each if block of } //else to make it clear, when reading the code, that the only intended execution after the if code block is if the condition in the if statement was false. Without something like this, particularly if the code within the if block grows to be more complex, it may not be clear to whoever is maintaining the code that the intent was to never execute past the if block when the if condition is true. – Makyen Dec 10 '16 at 22:01
  • @Makyen you raise a good point. One I was trying to ignore. I also believe something should be done after the if to make it clear that this section of code is done and the structure is finished. To do this I do what I haven't done here (because I didn't want to distract from the OP's main point by making other changes). I do the simplest thing I can that achieves all that without being distracting. I add a blank line. – candied_orange Dec 10 '16 at 22:21
27

I think it depends on the semantics of the code. If your three cases are dependent on each other, state it explicitly. This increases readability of the code and makes it easier to understand for anyone else. Example:

if (x < 0) {
    return false;
} else if (x > 0) {
    return true;
} else {
    throw new IllegalArgumentException();
}

Here you are clearly depending on the value of x in all three cases. If you'd omit the last else, this would be less clear.

If your cases are not directly depended on each other, omit it:

if (x < 0) {
    return false;
}
if (y < 0) {
    return true;
}
return false;

It's hard to show this in a simple example without context, but I hope you get my point.

6

I prefer the second option (separate ifs with no else if and early return) BUT that is as long as the code blocks are short.

If code blocks are long it's better using else if, because otherwhise you would not have a clear understanding of the several exits points the code has.

For example:

function doSomething(value) {
  if (check1(value)) {
    // ...
    // imagine 200 lines of code
    // ...
    return -1;
  }
  if (check2(value)) {
    // ...
    // imagine 150 lines of code
    // ...
    return value;
  }
  return false;
}

In that case, it is better use else if, store the return code in a variable and have just one return sentence at the end.

function doSomething(value) {
  retVal=0;
  if (check1(value)) {
    // ...
    // imagine 200 lines of code
    // ...
    retVal=-1;
  } else if (check2(value)) {
    // ...
    // imagne 150 lines of code
    retVal=value;
  }
  return retVal;
}

But as one should strive for functions to be short, I'd say keep it short and exit early as in your first code snippet.

  • 1
    I agree with your premise, but as long as we're discussing how code ought to be, might we as well not just agree code blocks should be short anyway? I think it is worse to have a long block of code not broken into functions than to use else if, if I had to choose. – kojiro Dec 10 '16 at 19:02
  • 1
    A curious argument. return is within 3 lines of if so it's not that different than else if even after 200 lines of code. The single return style you introduce here is a tradition of c and other languages that don't report errors with exceptions. The point was to create a single place to clean up resources. Exception languages use a finally block for that. I suppose this also creates a place to put a break point if your debugger wont let you put one on the functions closing curly brace. – candied_orange Dec 10 '16 at 19:23
  • 4
    I don't like the assignment to retVal at all. If you can safely exit a function do so immediately, without assigning the safe value to be returned to a variable of a single exit from the function. When you use the single return point in a really long function I don't generally trust other programmers and end up looking through the rest of the function for other modifications of the return value as well. Fail fast and return early are, among others, two rules I live by. – Andy Dec 10 '16 at 22:40
  • 2
    In my opinion it's even worse for long blocks. I try to exit functions as early as possible no matter what the outer context is. – Andy Dec 10 '16 at 22:45
  • 2
    I'm with DavidPacker here. The single return style forces us to study all assignment points as well as code after assignment points. Studying the exit points of a early return style would be preferable to me, long or short. So long as the language permits a finally block. – candied_orange Dec 10 '16 at 23:57
4

I use both in different circumstances. In a validation check, I omit the else. In a control flow, I use the else.

bool doWork(string name) {
  if(name.empty()) {
    return false;
  }

  //...do work
  return true;
}

vs

bool doWork(int value) {
  if(value % 2 == 0) {
    doWorkWithEvenNumber(value);
  }
  else {
    doWorkWithOddNumber(value);
  }
}

The first case reads more like you're checking all the prerequisites first, getting those out of the way, then progressing to the actual code you want to run. As such, the juicy code you really want to be running belongs in the scope of the function directly; it's what the function's really about.
The second case reads more like both paths are valid code to be run that are just as relevant to the function as the other based on some condition. As such, they belong in similar scope levels to each other.

0

The only situation I've ever seen it really matter is code like this:

List<?> toList() {
    if (left != null && right != null) {
        Arrays.asList(merge(left, right));
    }
    if (left != null) {
        return Arrays.asList(left);
    }
    if (right != null) {
        return Arrays.asList(right);
    }
    return Arrays.asList();
}

There is clearly something wrong here. The problem is, it's not clear if return should be added or Arrays.asList removed. You cannot fix this without deeper inspection of related methods. You can avoid this ambiguity by always using fully exhaustive if-else blocks. This:

List<?> toList() {
    if (left != null && right != null) {
        Arrays.asList(merge(left, right));
    } else if (left != null) {
        return Arrays.asList(left);
    } else if (right != null) {
        return Arrays.asList(right);
    } else {
        return Arrays.asList();
    }
}

won't compile (in statically checked languages) unless you add explicit return in first if.

Some languages do not even allow the first style. I try to use it only in strictly imperative context or for preconditions in long methods:

List<?> toList() {
    if (left == null || right == null) {
        throw new IllegalStateException()
    }
    // ...
    // long method
    // ...
}
-2

Having three exits from the function like that is really confusing if you are trying to follow the code through and bad practice.

If you have a single exit point for the function then the elses are required.

var result;
if(check1(value)
{
    result = -1;
}
else if(check2(value))
{
    result = value;
}
else 
{
    result = 0;
}
return result;

In fact lets go even further.

var result = 0;
var c1 = check1(value);
var c2 = check2(value);

if(c1)
{
    result = -1;
}
else if(c2)
{
    result = value;
}

return result;

Fair enough you could argue for a single early return on input validation. Just avoid wrapping the whole bulk if your logic in an if.

  • 3
    This style comes from languages with explicit resource management. Single point was needed to free up allocated resources. In languages with automatic resource management single return point is not that important. You should rather focus on reducing number and scope of variables. – Banthar Dec 11 '16 at 15:56
  • a: no language is specified in the question. b: I think you are wrong on this. there are plenty of good reasons for single return. it reduces complexity and enhances readability – Ewan Dec 11 '16 at 16:00
  • 1
    Sometimes it reduces complexity, sometimes it increases it. See wiki.c2.com/?ArrowAntiPattern – Robert Johnson Dec 12 '16 at 12:26
  • No I think it always reduces cyclomatic complexity, see my second example. check2 always runs – Ewan Dec 12 '16 at 12:29
  • having the early return is an optimisation which complicates the code by moving some code into a conditional – Ewan Dec 12 '16 at 12:30
-3

I prefer omitting the redundant else statement. Whenever I see it (in code review or refactoring) I wonder if the author understood the control flow and that there might be a bug in the code.

If the author believed the else statement was necessary and thus did not understand the control flow implications of the return statement surely such a lack of understanding is a major source of bugs, in this function, and in general.

  • 3
    While I happen to agree with you this isn't an opinion poll. Please back up your claims or the voters won't treat you kindly. – candied_orange Dec 10 '16 at 21:23

protected by gnat Dec 13 '16 at 12:59

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.