Should "else" be used in situations where control flow renders it redundant?

Question

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.

Answer 1

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.

Answer 2

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.

Answer 3

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.

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

Answer 5

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

Answer 6

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.

Answer 7

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.