If-else ladder that is supposed to catch all conditions - should a redundant final clause be added?

Question

This is a thing I'm doing a lot lately.

Example:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else if (i > current) {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }
}

Oftentimes the if/else ladder is significantly more complicated than this...

See the final clause? It is redundant. The ladder is supposed to ultimately catch all possible conditions. Thus it could be rewritten like that:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }
}

This is how I used to write code, but I dislike this style. My complaint is that the condition under which the last part of code will be executed is not obvious from the code. I thus started writing this condition explicitly to make it more evident.

However:

Explicitly writing the final exhaustive condition is my own idea, and I have bad experiences with my own ideas - usually people scream at me about how horrible what I'm doing is - and (sometimes much) later on I find out that it was indeed suboptimal;
One hint why this may be a bad idea: Not applicable to Javascript, but in other languages, compilers tend to issue warnings or even errors about control reaching end of function. Hinting doing something like that might be not too popular or I'm doing it wrong.
- Compiler complaints made me sometimes write the final condition in a comment, but I guess doing so is horrible since comments, unlike code, have no effect on actual program semantics:

    } else { // i > current
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }

Am I missing something? Or is it OK to do what I described or is it a bad idea?

Christophe · Answer 1 · 2019-09-21T22:18:58.610

Both approach are valid. But let's have a closer look at pros and cons.

For an if-chain with trivial conditions like here, it doesn't really matter:

with a final else, it is obvious for the reader to find out in which condition the else is triggered;
with a final else if, it is as obvious for the reader that no additional else is needed since you covered it all.

However, there are plenty of if-chains that rely on more complex conditions, combining states of several variables, perhaps with a complex logical expression. In this case it is less obvious. And here the consequence of each style:

final else: you're sure that one of the branch is taken. If it happen that you've forgotten one case, it will go through that last branch, so during debugging, if the last branch was chosen and you expected something else, you'll quickly figure out.
final else if: you need to derive the redundant condition to code, and this creates a potential error source with the reisk of not covering all the cases. Furthermore, if you've missed a case nothing will be performed and it could be more difficult to find out that something was missing (e.g. if some variables that you expected to be set keep values from previous iterations).

So the final redundant condition is a source of risk. This is why I'd rather suggest to go to a final else.

Edit: high reliability coding

If you are developing with high reliability in mind, you may be interested in another variant: completing your redundant explicit final else if with a final else in order to catch any unexpected situations.

This is defensive coding. It's recommended by some security specifications such as SEI CERT or MISRA. Some static analysis tools even implement this as a rule that is systematically checked (this could explain your compiler warnings).

Loren Pechtel · Answer 2 · 2019-09-22T15:58:54.863

Something that is missing from the answers so far is a matter of what sort of failure is less harmful.

If your logic is good it doesn't really matter what you do, the important case is what happens if you have a bug.

You omit the final conditional: The final option executes even if that's not the right thing to do.

You simply add the final conditional: It doesn't execute any option, depending on the situation this might simply mean something fails to display (low harm), or it might mean a null reference exception at some later point (which could be a debugging pain.)

You add the final conditional and an exception: It throws.

You have to decide which of these options is the best. In development code I consider this a no-brainer--take the third case. However, I probably would set circle.src to an error image and circle.alt to an error message before throwing--in case someone decides to turn off the assertions later this makes it fail harmlessly.

Another thing to consider--what are your recovery options? Sometimes you don't have a recovery path. What I think is the ultimate example of this is the first launch of the Ariane V rocket. There was an uncaught /0 (actually a division overflow) error that resulted in the destruction of the booster. In reality the code that crashed served no purpose whatsoever at that point, it had become moot the instant the strap-on boosters lit. Once they light it's orbit or boom, you do the best you can, errors can't be permitted. (If the rocket goes astray due to this the range safety guy turns his key.)

Lie Ryan · Answer 3 · 2019-09-22T00:37:51.380

What I recommend is using an assert statement in your final else, in either of these two styles:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else {
        assert i > current
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }
}

Or a dead code assertion:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else if (i > current) {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    } else {
        assert False, "Unreachable code"
    }
}

Code coverage tool can often be configured to ignore code like "assert False" from coverage report.

By putting the condition in an assertion, you effectively document the condition of a branch explicitly, but unlike a comment, the assertion condition can actually be checked and will fail if you keep assertions enabled during development or on production (I generally recommend keeping assertions enabled in production if they don't affect performance too much).

score 0 · Answer 4 · answered Sep 24 '19 at 12:29

I defined a macro “asserted” which evaluates a condition, and in a debug build falls into the debugger.

So if I’m 100 percent sure that one of three conditions must be true, I write

If condition 1 ...
Else if condition 2 .,,
Else if asserted (condition3) ...

That makes it clear enough that one condition will be true, and no extra branch for an assert is needed.

score -2 · Answer 5 · answered Sep 23 '19 at 23:07

I recommend avoiding else entirely. Use if to declare what the block of code is supposed to handle, and end the block by exiting the function.

This results in code that is very clear:

setCircle(circle, i, { current })
{
    if (i == current)
    {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
        return
    }
    if (i < current)
    {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
        return
    }
    if (i > current)
    {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
        return
    }
    throw new Exception("Condition not handled.");
}

The final if is of course redundant... today. It can become very important thought if/when some future developer re-arranges the blocks. So it is helpful to leave it in there.

If-else ladder that is supposed to catch all conditions - should a redundant final clause be added?

5 Answers5