0

I am writing some code that enables / disables a certain kind of hardware function. To enable it on, I have to call some methods, and to disable it, some others.

Of course I want this code to be clean, so I try to keep the use of if/else to a minimum. See Elegant ways to handle if(if else) else and Clarification of "avoid if-else" advice for discussions about this.

void LightTest(bool enable) {
    if (enable) {
        TurnOnAllLights();
        SetValueOnAllLights(1);
    } else {
        TurnOffAllLights();
    }
}

However, this seems like a case, where I cannot avoid using else. The question is, is this true?

12
  • 19
    "I want this code to be clean, so I try to keep the use of if/else to a minimum." If applied mindlessly, this is a silly rule. Feb 8, 2022 at 13:56
  • @PhilipKendall hence my question, to find out where the borders of this rule are. To add some mindfulness if you want :-). Feb 8, 2022 at 13:58
  • 15
    You're asking the wrong question IMO. It shouldn't be "some guy in a book said else is not clean code, can I ignore the rule?", it should be "is this code easy to understand and maintain?" Feb 8, 2022 at 14:04
  • 4
    Maybe a better way to express @PhilipKendall's point: it would be more productive, instead of trying to figure out where the line is for this particular advice, to address why you're trying to dogmatically adhere to it in the first place. In the end, the only goal is "is this readable? Can it be better?", not "Was this else really necessary?". To that extent, the question you posted here focuses on the wrong approach. Even if we answer your direct question, it doesn't help with any and all other advice that you receive and dogmatically try to apply, yet the real answer is always the same.
    – Flater
    Feb 8, 2022 at 14:53
  • 1
    Thanks all. I have to note I do not want to dogmatically follow this rule, but just have it in my head to trigger where I can improve (clean) my code. My question is exactly about when not to use it. Forgive me if I have given you the impression that I blindly follow this kind of rules (I tend to not). I strive for understanding them and sites like this help me in getting a better software engineer. Feb 8, 2022 at 15:43

2 Answers 2

14

This is a misleading assumption. Removing else statement does not make the code cleaner. It just increases the risk of mistakes.

For a detailed discussion, see for example the recommendation MSC01-C from the CERT C secure coding standard, which explains that the code should strive for logical completeness, as vulnerabilities can result when failing to consider all the possible alternatives. In this regard, the else visibily demonstrates that you have considered them.

Proof by contradiction

Let's have a look at your code, and see how you could avoid the else. The systematic approach is to llok at the possible execution graphs and regorganize them to produce an equivalent graph.

A first approach is to add a conditional for the contrary:

if (enable) {
    TurnOnAllLights();
    SetValueOnAllLights(1);
} 
if (!enabled) {  // ====> OUCH ! super risky 
    TurnOffAllLights();
}

Rewriting the code like that removes the else, but it is super risky. Not in your small snippet, but in real-world if statements, in the case where the first if-block would change the value of enabled, which will result in both blocks being run instead of one of the two.

A second approach is to use a premature return

if (enable) {
    TurnOnAllLights();
    SetValueOnAllLights(1);
    return;   // ===> HORRIBLE IN THIS CASE
} 
TurnOffAllLights();

It's not always bad to prematurely return. But in this case it is it is: it requires more brainpower to understand that it's just about an alternative. Moreover, you may multiply unnecessarily function exit points, which is not a very popular nor effective practice either. Furthermore, how would you do if you'd still have a common part of code to run after the else? Copy-paste it and repeat yourself with all the risks that it represents for the future?? This is especially tricky if this additional ode would be added later, bearing the significant risk of forgetting it in one of the branch.

Now the finale, the return to the good old GOTO; really?

if (enable) {
    TurnOnAllLights();
    SetValueOnAllLights(1);
    goto GREAT_SPAGHETTI_CONTINUES;
} 
TurnOffAllLights();
GREAT_SPAGHETTI_CONTINUES: // OUCH! 
//...
}

I'll not analyse this snippet, but just remind that it took a decade to get rid of problem-solving-by-adding-another-goto, to finally discover the virtues of structured programming. Let's not go back to wild-goto age. Again, there are cases, where the use of goto could be discussed, but avoiding the else definitively isn't.

If you could change more than the control flow

I cannot resist but to delegate the setting of the light to a single function:

TurnAllLights(enabled); // incl.responsibility for setting value correctly 
10
  • 1
    if (!enabled) { // ====> OUCH ! super risky <- not really risky, it won't compile as you've written enabled instead of enable :-) Feb 8, 2022 at 15:38
  • 1
    Thanks for your lengthy explanation. About that last one, that only pushes the problem to the TurnAllLights method and is not a solution if you ask me. Feb 8, 2022 at 15:40
  • 2
    Another popular way of removing conditionals is to replace them with polymorphism - of course, for this example, this is another approach of making the code less readable,
    – Doc Brown
    Feb 8, 2022 at 16:56
  • Resist Feb 8, 2022 at 17:05
  • 1
    @BartFriederichs yes, sorry for the length. I took care to write the essentials in the first two paragraphs. The proof by contradiction is more to reinsure you about the virtue of the else. I expected your remark on the TurnAllLights() and I guess that your real case is more complex. I just couldn't resist ;-)
    – Christophe
    Feb 8, 2022 at 18:38
0

If it's a pure on/off scenario, a function called toggleLights() would be sufficient instead of turnon/turnoff. There wouldn't be a need to pass around a boolean. In the value for lights set value the to the inverse like:

x = !x;

That should be sufficient and there would not be a need to have a conditional. Now if you needed to do some additional logic based on the new value of the setting, maybe then you would need a conditional to inspect that current value.

1
  • 1
    1. If this interfaces with hardware (as OP does), the software would need to cache the value internally to allow for a toggleLights() function, which is not ideal (as any sort of caching can get out of sync). Looking it up can be costly, and it always complicates the calling code. 2. Quite often the functions using this API would want to set the state without caring for the previous value - in this case toggleLights() is also inappropriate. 3. In general, while I appreciate frame challenges, this one seems inappropriate.
    – jaskij
    Feb 10, 2022 at 2:42

Your Answer

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

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