Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
3  
What is the security problem for you? What does "passwordCheck" mean? There was a password check? There needs to be a password check? The user has passed? The user has failed to enter the right password? –  LennyProgrammers Dec 20 '10 at 11:16
20  
I know that passwordCheck is likely to be a boolean... What do you mean? In any strong-typed language. passwordCheck will be whatever you want it to be. –  Bobby Dec 20 '10 at 11:23
30  
I think bad indentation practices lead to more errors than using else statements... –  gablin Dec 20 '10 at 12:43
15  
This seems odd. First, you complain about the possible return type of passwordCheck() possibly not being boolean (which may be a reasonable concern), and then you blame it on else? I don't see what problems the else causes. –  David Thornley Dec 20 '10 at 15:21
7  
mmm, i think asking if using else is bad programming is bad programming –  Muad'Dib Dec 20 '10 at 17:55
show 15 more comments

closed as not a real question by gnat, Jim G., ChrisF Aug 23 '12 at 20:59

It's difficult to tell what is being asked here. This question is ambiguous, vague, incomplete, overly broad, or rhetorical and cannot be reasonably answered in its current form. For help clarifying this question so that it can be reopened, visit the help center.If this question can be reworded to fit the rules in the help center, please edit the question.

13 Answers

up vote 85 down vote accepted

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.

share|improve this answer
2  
@dave.b in the context of the example, I guess it would be a security issue, but if this is all over the place in the code base you're looking at, it's more a sign of whoever wrote it needing a bit more practice :) –  RYFN Dec 20 '10 at 11:28
8  
Can someone elaborate on the security aspect of this? if(!something){do a}else{do b} versus if(something){do b}else{do a} is logically equivalent no? I am trying to understand what the difference is in terms of security of this? –  Chris Dec 20 '10 at 13:05
11  
@Chris: I think the OP uses a weakly-typed language. Therefore passwordCheck could be anything, f.e. null, which would render passwordCheck == false to false and would the user allow to login because of an internal error. –  Bobby Dec 20 '10 at 13:38
1  
@Chris, my understanding was that the default state was to allow access, which is not necessarily advisable. –  RYFN Dec 20 '10 at 14:10
3  
Yes, this is very language dependent. In C#, where if requires a bool, variables must be definitely assigned, all paths must return a value, etc., I cannot think of any reason the order of if and else would matter besides readability. That is, if(a){b();}{c();} should be equivalent to if(!a){c();{b();}. In JavaScript, on the other hand, you have to be aware that passwordCheck could be undefined, etc. –  Tim Goodman Dec 20 '10 at 20:35
show 2 more comments

Then just replace them,

If (passwordCheck == true)
{
     letThemIn();
}
else
{
     displayMessage();
}
share|improve this answer
18  
How about If ((passwordCheck == true) == true) ? :-) –  Hippo Dec 20 '10 at 11:26
1  
Well it's my style, to improve readability. You may write if(!value) but i prefer if(value != true) –  Ahmet Kakıcı Dec 20 '10 at 11:32
5  
It doesn't improve readability. It just adds noise. Even worse are programmers who write nested if constructs just because they're scared of the Boolean operators. –  ak2 Dec 20 '10 at 14:54
1  
If the variable name is descriptive, there's no reason why: if (someBooleanValue == true) should be necessary. This would be better: if (validPassword) {... –  Mark Freedman Dec 20 '10 at 15:28
    
Well i just replaced the code blocks within the question. If you read my first comment i said that i prefer using if(value!=true) instead of if(!value). I hope you can see the difference between if(value) and if(!value). I meant i don't use complement operator. Otherwise you are right, true means true. –  Ahmet Kakıcı Dec 20 '10 at 17:17
show 1 more comment

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.

share|improve this answer
5  
-1. This answer is too ridiculous. I am not saying its not true. It just misses the point. What the compile generates from your code has nothing to do with coding practices and bugs you write –  acidzombie24 Dec 20 '10 at 20:02
2  
The questions was "Is using ELSE bad programming?". My answer is: you can pretend it's not there, but not avoid it. –  LennyProgrammers Dec 21 '10 at 10:20
4  
what language ? in Java the else is not in the bytecode :P –  IAdapter Jan 9 '11 at 20:28
    
@acidzombie24 - it's very good practice to consider what the 'else' is from any question. Sometimes it's just - do everyhtign else in the fucntion, but it does show you have thought about it –  Martin Beckett Aug 23 '12 at 17:12
add comment

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.

share|improve this answer
11  
I agree with this. Copy-pasting an if statement and inverting its boolean expression just to avoid an else -- now that is bad programming! Not only are you duplicating code (bad programming), you're also slowing down performance (if the check is really complicated, you're now doing it twice -- ba-- uh, well, you know what they say about premature optimizations nowadays... not so good programing!). –  gablin Dec 20 '10 at 12:46
    
To be honest that If check should be in a method of its own to return true / false –  billy.bob Dec 20 '10 at 12:57
    
Good examples, lets not forget the performance of using 2 if checks versus one if paired with an else. I would assume in most languages using an if/else will perform better than using two if statements with one using a not on the condition. –  Chris Dec 20 '10 at 13:07
1  
dave.b: sure, but it could start simple and grow slowly; the complex check in my example is here to make the problem more obvious. –  user281377 Dec 20 '10 at 13:39
2  
Yup - and don't forget that conditions can have side effects in most languages, and if there's functions in the conditions you really can't tell by looking. –  David Thornley Dec 20 '10 at 15:19
add comment

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.
share|improve this answer
2  
I would also include "Move complex boolean checks into a separate function". –  gablin Dec 20 '10 at 12:41
add comment

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.

share|improve this answer
    
I agree. "else" creates a mental "fork" and we humans are sequential creatures. –  user187291 Dec 20 '10 at 17:13
    
Fyi, it's called a guard condition –  CaffGeek Dec 20 '10 at 19:51
    
@Chad: ah thanks, always nice to get name for things :) –  Matthieu M. Dec 20 '10 at 20:04
1  
+1. This is the only answer i can stand that has a score of >1. *writes an answer* –  acidzombie24 Dec 20 '10 at 20:05
    
I do not try to avoid else as such, but I think I agree with what you write. The point is that the thing you are testing should be the exception and not the normal case (stackoverflow.com/questions/114342/…). Here authentication failure is the exception and (only) that should be tested. –  hlovdal Dec 20 '10 at 22:58
show 6 more comments

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.

share|improve this answer
add comment

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

share|improve this answer
1  
as a side note: if you have an if construct in your pure functional language, then you really need to have an else. Every expression has to return something! –  tokland Dec 20 '10 at 15:11
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
    
I didn't know that there were languages that returned more than one datatype! Are you refering to polymorphic functions? I believe whenever I overload a function it has always returned the same datatype, although it may take different arguments. –  Michael K Dec 20 '10 at 14:46
1  
In loosely typed languages, and especially in PHP, functions can return more than one data-type. eg: stristr-"Returns the matched substring. If needle is not found, returns FALSE" –  Craige Dec 20 '10 at 14:50
    
@Michael, A PHP function can return anything you want it to. There's no restriction on the datatype. My most complicated function returns true/false/null, but there's nothing (except common sense) stopping you writing a function which returns true/integer/null/string/float/array. –  TRiG Dec 20 '10 at 18:00
    
Interesting. I've never worked with PHP. Thanks for the explanation though - probably help me in the future! sort of like Javascript then? –  Michael K Dec 20 '10 at 18:01
    
@Michael - Quite similar to Javascript indeed. –  Craige Dec 20 '10 at 19:08
add comment

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()
}
share|improve this answer
add comment

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

share|improve this answer
add comment

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.

share|improve this answer
add comment

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