10

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?

Improve this question
3
  • +1 for @Christophe's execllent answer that redundancy in code increases chances for defects. There's also a much lesser efficiency issue. To expand on that, regarding the code example in question, we could write the series of if-else-if's as just separate if's without any elses, but we generally wouldn't since the if-else's give the reader the notion of exclusive alternatives rather than series of independent conditions (which would also be less efficient, since that says that all the conditions should be evaluated even after one matches).
    – Erik Eidt
    Sep 21, 2019 at 22:44
  • @ErikEidt > since that says that all the conditions should be evaluated even after one matches +1 unless you are returning from a function or "breaking" from a loop (which is not the case in the above example).
    – Darek Nędza
    Sep 22, 2019 at 12:08
  • 1
    +1, Nice question. As an aside it might interest you that in the presence of NaNs the example is not exhaustive.
    – monocell
    Sep 22, 2019 at 16:29

5 Answers 5

Reset to default
6

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

Improve this answer
6
  • 7
    What if after the final redundand condition I add an else that always throws an exception which says "You were not supposed to reach me!" - Will it alleviate some of the problems of my approach?
    – gaazkam
    Sep 21, 2019 at 21:45
  • 3
    @gaazkam yes, this is a very defensive style of coding. So you still need to compute the final condition, but for complex error-prone chains, you'd at least find out quickly. The only issue I see with this variant is that it's a little overkill for obvious case.
    – Christophe
    Sep 21, 2019 at 21:52
  • @gaazkam I've edited my answer to address also your additional idea mentioned in your comment
    – Christophe
    Sep 21, 2019 at 22:19
  • 1
    @gaazkam Throwing an exception is good. If you do, do not forget to include the unexpected value in the message. It may be hard to reproduce and the unexpected value may provide a clue about the source of the problem.
    – Martin Maat
    Sep 22, 2019 at 6:33
  • 1
    Another option is to put an assert in the final else if. Your style guide may vary on whether that is a Good Idea, however. (The condition of an assert should never be false, unless the programmer screwed up. So this is at least using the feature for its intended purpose. But so many people misuse it that a lot of shops have banned it outright.)
    – Kevin
    Sep 22, 2019 at 20:04
5

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

Improve this answer
4

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

Improve this answer
1
  • 1
    I don't like your first option, the second is much clearer that it's a should-be-impossible case.
    – Loren Pechtel
    Sep 22, 2019 at 5:40
0

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.

Improve this answer
-2

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.

Improve this answer
4
  • 1
    It’s actually very unclear because now I need to consider whether executing the first branch might change the outcome of the second test. Plus pointless multiple returns. Plus possibility that no condition is true.
    – gnasher729
    Sep 24, 2019 at 12:31
  • I actually write if statements like this when I am intentionally expecting multiple cases can be taken. It's especially useful in languages with switch statements that don't allow fall through. I think if the choices are mutually exclusive it should be written so that it is obvious that the cases are mutually exclusive (using else). And if it isn't written like that then the implication is that the cases aren't mutually exclusive (fall through is possible).
    – Jerry Jeremiah
    Sep 24, 2019 at 22:09
  • @gnasher729 I totally understand your reaction. I know some very smart people who had trouble with "avoid else" and "early return"... at first. I encourage you to take some time to get used to the idea and look into it-- because these ideas do, objectively and measurably, reduce complexity. The early return actually addresses your first point (needing to consider whether a branch might change another test). And the "possibility that no condition is true" still exists regardless; with this style, you get an obvious exception rather than a hidden logic flaw.
    – John Wu
    Sep 24, 2019 at 23:38
  • But isn't cyclomatic complexity of both styles precisely the same? ie equal to the number of conditions
    – gaazkam
    Sep 25, 2019 at 21:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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