Is using ELSE bad programming? [closed]

Question

I've often come across bugs that have been caused by using the ELSE construct. A prime example is something along the lines of:

If (passwordCheck() == false){
    displayMessage();
}else{
    letThemIn();
}

To me this screams security problem. I know that passwordCheck is likely to be a boolean, but I wouldn't place my applications security on it. What would happen if its a string, int etc?

I usually try to avoid using ELSE, and instead opt for two completely separate IF statements to test for what I expect. Anything else then either gets ignored OR is specifically handled.

Surely this is a better way to prevent bugs / security issues entering your app.

How do you guys do it?

Answer 1

The else block should always consist of what you want the default behaviour to be.

There's no need to avoid them, just be careful to use them appropriately.

In your example, the default state should be to not allow access. A little refactoring leaves you with:

If (passwordCheck)
{
   letThemIn();
}
else
{
   displayMessage();
}

i.e. if the password check works, let them in, otherwise it's always valid to show some error message.

You can of course add additional checks to you logic by using else if rather than completely separate if statements.

Answer 2

No, there is nothing wrong with ELSE. ELSE is not the new GOTO. In fact, using two IFs instead of ELSE can lead to several problems.

Example one:

if (this(is(a(very())==complex(check())))) {
   doSomething();
}

if (!this(is(a(very())==complex(check())))) {
   doTheOtherThing();
}

You see the copy-paste? It just waits for the day when you change one and forget the other.

Example two:

foreach(x in y) {
  if (first_time) {
    first_time = false;
    doSomething();
  }

  if (!first_time) {
    doTheOtherThing();
  }
}

As you can see, the second IF will also be executed for the first item, because the condition has already changed. In real world programs, such bugs are harder to spot.

Answer 3

There is always an ELSE. If you write

if(foo)
  bar();

you actually write

if(foo)
{
  bar();
}
else
{
   // do nothing
}

Whatever you put in the ELSE-path is your responsibility.

Answer 4

I personally tend to avoid else as much I can, but it's not for any security problem.

When reading code, nested statements make it harder to follow the logic, because you need to remember which set of conditions will lead there. For this reason, I am a huge fan of early exit:

if (checkPassword != OK) { displayMessage(); return; }

letThemIn();

This also applies to for and while loops in which I will use continue and break whenever it avoids a level of indentation.

Chris Lattner says it better than I do in the LLVM Coding Standards.

Answer 5

Then just replace them,

If (passwordCheck == true)
{
     letThemIn();
}
else
{
     displayMessage();
}

Answer 6

Like Matthieu M., I prefer early exit to deeply nested else blocks... It illustrates well defensive programming (if bad conditions, no point to continue). Lot of people will disagree with us, preferring a unique exit point; it is not the point of the debate (I think).

Now, I certainly use else when it makes sense, particularly for simple, short alternatives. As said, duplicating the test is a waste of time (programmer's and CPU's), a source of confusion and, later, of bugs (when one is changed, not the other).
Sometime, I add a comment on the else part, reminding what was the condition (particularly if the if part is long, eg. in legacy code) or what is the alternative.

Note that some extreme proponents of functional programming propose to get rid entirely of if, in favor of pattern matching... A bit too extreme for my taste. :-)

Answer 7

There is nothing wrong with using ELSE. However it can lead to overly complex code that is hard to read and understand. It may indicate a bad design. It certainly indicates additional use cases that will need to be tested.

Try to remove ELSEs if you can - but don't be paranoid about it. Steve McConnell calls this straight line code in Code Complete. I.e. there is a simple clear path through your code.

Approachs to try for your particular problem:

• use polymorphism. At the boundary to your system validate the security credentials of the user. If they are legitimate then return a session object - with access to the relevant parts of the system or throw an exception. However this could make you system more complex. So you decide what's easier to understand and maintain.

In general - the following may help to reduce ELSEs in your code:

• good requirements may reduce the need for such decisions in the code. You may not need to implement the use case (else) at all.
• clearer design. Maximum cohesion and minimize coupling. This ensures components are not duplicating decisions made in other components.
• exception handling to manage error cases.
• polymorphism (see example above).
• switch statements - these are glorified ELSEs but are better in certain situations.

Answer 8

Your supposition about the code being a security leak may or may not be true depending on the language you are using. In C code it could be a problem (particularly because in C a boolean is just an int that is non-zero or zero)--but in most strongly typed languages (i.e. runtime type checking) if the passwordCheck variable was declared as a boolean, there is no way to assign something else to it. In fact, everything in an if predicate must resolve to a boolean, whether you use the boolean operators or simply use the value. If you managed to have another type of object bound to passwordCheck the runtime would throw some type of illegal cast exception.

Simple if/else constructs are much easier to read than if/if constructs--and less prone to inadvertant problems if someone tries to flip the construct. Let's take the same example for a second:

if(passwordCheck == false) {
    denyAccess();
}

if(passwordCheck) {
    letThemIn();
}

The meaning of the mutually exclusive clauses you want to execute above is lost. That's what the if/else construct conveys. Two mutually exclusive branches of execution, where one of them will always run. This is an important part of security--ensuring there is no way to letThemIn after you've called denyAccess.

For the purpose of code clarity, and for the purpose of ensuring critical sections are most protected, they should be inside the primary clause (the if part). The default non-compliant behavior should be in the alternate clause (the else part). For example:

if(passwordCheck) {
    letThemIn();
} else {
    denyAccess();
}

NOTE: in working with different languages, I've developed a coding habbit that does help avoid the question of "what if it's a string?" Essentially, it is to put the constant first in the boolean expression. For example, instead of checking passwordCheck == false I am checking false == passwordCheck. This also avoids the accidental assignment problem possible in C++. Using this approach, the compiler will complain if I type = instead of ==. In languages like Java and C#, the compiler would treat the assignment in the if clause as an error, but C++ will happily accept it. That's why I also tend to do null checking with the null first.

If you routinely change languages placing the constant first is very helpful. However, on my team it is opposite the coding standard and the compiler catches those problems anyway. It can be a hard habbit to break.

Answer 9

Saying that using else when programing is bad is like saying that using otherwise when speaking is bad.

Sure, they can both be used in bad ways, but that doesn't mean they are to be avoided just because you made a mistake which happened to include them. I wouldn't be surprised if many bugs depended on a missing default case in a switch statement.

Answer 10

Think of Else as white-listing your application flow. You check for conditions that SHOULD allow application flow to continue, and if these are not met, then your Else is executed to either resolve the problem, discontinue the application execution, or something similar.

Else in itself is not bad, but if you use it poorly, you can see undesired effects.

Also, in regards to your statement about

"I know that passwordCheck is likely to be a boolean, but I wouldn't place my applications security on it."

For methods that you develop, ALWAYS return one data type. Although PHP Core is littered with code that returns two or more datatypes, this is a bad practice as it makes guesswork of function calls. If you have to return more than one datatype, consider throwing an exception (I find this is often the reason I would want to return another data type - something went horribly, horribly wrong), or consider re-structuring your code so that you can return only one data-type.

Answer 11

First of all. LOL! Theres NO REASON to avoid else, at all. Its NOT bad practice in any way, shape or form.

If anything the code should be

if(!IsLoggedIn) { ShowBadLoginOrNoAccessPage(); return }

Theres no two ifs there and it has no else. This is what i do in all my apps except one in which i throw an exception. The exception is caught in my function which checks the url for the proper page to show (or alternatively i can put the catch/check in asp.net error function). It prints out a generic page that says not authorize or whatever message i use in the exception (i always check the type of exception and set the http status code).

-Edit- as shown in ammoQ example two ifs is ridiculous. Really else is just as good or better then an if. If anything ifs are to be avoided (although i personally dont. But i do use return and break a lot) as its been said more code paths increase the likelihood of bugs. See Cyclomatic Complexity

-Edit 2- If you are worried about if/else usage. I'll also note that my preference is to put the shortest code block at top such as

if(cond == false) {
    a()
    b()
    c()
    onetwothree()
}
else
{
    a()
    b()
    c()
    more()
    onetwothree()
    code()
    longer()
}

Rather then

if(cond) 
{
    a()
    b()
    c()
    more()
    onetwothree()
    code()
    longer()
}
else
{
    a()
    b()
    c()
    onetwothree()
}

Answer 12

I would argue that using branching logic of any kind should be avoided in as much as possible. While nothing is wrong with ELSE or IF, there are lots of ways to write code to minimize the need to use any branching logic. I'm not saying branching logic can be totally eliminated--it will be needed in some places--but it is possible to refactor code to eliminate a good chunk of it. In most cases this will improve the intelligibility and accuracy of your code.

As one example, ternary operators are also usually good candidates:

If VIP Then 
  Discount = .25
Else
  Discount = 0
End If
Total = (1 - Discount) * Total

Using a ternary approach:

Discount = VIP ? .25 : 0
Total = (1 - Discount) * Total

Ternary operators shift the branching to the right in a good way.

Answer 13

I like to set a default prior to conditionals when I can. I feel like it's a little easier to read and a bit more explicit, but this is just a preference. I have a tendency to try and avoid negative conditions in my code. I'm not a big fan of checking for !foo or false == foo and I feel like else is kind of the conditional equivalent of a negative.

foo = bar;

if ('fubar' == baz) {
    foo = baz;
}

instead of ...

if ('fubar' == baz) {
    foo = baz;
} else {
    foo = bar;
}

The previous code block just seems a little easier for me to read. It seems more natural to me to have a sort of skeptical paranoia about my code. Setting a default regardless of any condition makes me feel comfortable :P