Else Considered Smelly

This is about "else" like in 

 if (condition()) {
ifBlock();
 }
 else {
elseBlock();
 }
and not about anyone with the given name Else. Big excuses to all those!

"Smelly" is solely meant in the sense of program code that calls for improvement!

First smell:
When the condition() is the least bit complicated, the else is twice as complicated, because the reader has to invert the condition().

Second smell:
When the ifBlock() contains a little more than reasonable number of lines it is easy to forget what the condition() was.

Third smell:
Nested if-if-else, if-else-if (ArrowAntiPattern), etc. are likely to confuse the reader even more. Nested if's without any else's are not so bad.

Isn't that just good programming practice in general - to use the appropriate type of flow control construct (i.e. switch or exceptions) rather than putting everything into an if test? -- ChrisBaugh

[See "Discussion of Else Smells" below.]

Discussion of Else Smells:

First smell -- complicated condition():
A: Use DecomposeConditional to give the condition a meaningful name. Then "not that condition" will be easy to understand.

Second smell -- ifBlock() has too many lines:
A: Use ExtractMethod; problem solved.
Probably document the condition in an assert at the top of the extracted method, too.

Third smell -- nested if statements:
A: Nested if statements and loops may be confusing, with or without else; use ExtractMethod to simplify the code.

So, there's nothing really wrong with "else" at all: It's just that "else" happened to be seen where other code smells were present.
"Else" still adds the complexity of inverting the condition. This is specific to "else". It makes smell one and two worse and smell three much worse. cf. also next discussion point.

Both first and second smells are trivial to fix, if you just have the Then and Else clauses consist solely of a call to a subroutine. 
 if (condition()) {
   do_true_handling();
 } else {
   do_false_handling();
 }
 -- ChrisBaugh
I suppose you could say that else SmellsLikeBoiledOnions? - It doesn't smell bad on its own, but it makes almost any bad smell worse.

Possible Solutions:

To clarify: this is not replacing one line of code by a method call (that would be stupid). It is about replacing a possibly large block. It covers smell two and three.
[Discussion of each below.]

[Consider also ElseConsideredHelpful.]

Proposed Solution: Use a GuardClause, if one of the blocks is a normal case and the other one is an exceptional case.

ChuckMoore uses this idiom in ColorForth, which doesn't have else at all: 

 : doCondition   condition if ifBlock ; then elseBlock ;
Converted to C (for the benefit of those unfamiliar with ForthLanguage): 

 void doCondition() {
   if (condition()) {
     ifBlock();
     return;
   }
   elseBlock();
 }
Every such test is factored into its own subroutine. The ForthLanguage strongly favors factoring, with its low subroutine overhead. The same thing might not make sense for C and other high-level languages. (And the C version looks a lot yuckier than the Forth version anyway.)

The benefit to this is that there is no branch from the end of the if part around to the end of the else clause. (Chuck is really big on minimizing code.)

Another benefit is that every if is in its own subroutine, so it is easier to analyze things to ensure that the right branches are being taken. And you have to come up with a meaningful name for the routine: if you can't come up with a good name, then you might want to think about whether you really know what you're testing.

I don't think Chuck Moore would consider this to be a case of GuardClause. This is how he does all of his conditionals: the first clause is not necessarily an exceptional or rare case. Instead, he does it this way because he's a big proponent of DoTheSimplestThingThatCouldPossiblyWork, and he sees no need to have ELSE in his language.

Putting every "if" into its own method is an interesting idiom. It is similar, perhaps, to the LISP idiom that all looping should be done with tail recursion -- refactoring each loop into its own function. (...a pair of concepts I just expanded in LanguageIdiomsEncouragingSmallMethods.)

(BTW, ChuckMoore also favors Scheme-like tail-recursion for loops in Forth.)

But, especially when looking at the "C" example above, I don't see how "return;" makes the code more readable than "else": An "else" very clearly says to the reader, "we'll do the code above or below the 'else,' but not both." -- JeffGrigg

Agreed. The C version is ugly and less readable than using "else". But the ColorForth version is simple, and users would be familiar with the idiom.

Outside FORTH, it may be that ElseConsideredHelpful. See below.

Outside FORTH, it may be that ElseConsideredHelpful.

No, it's not. ANSI Forth programmers will often make use of ELSE. Just as CommonLisp strongly prefers iterative, rather than recursive programming styles as encouraged by SchemeLanguage, so too does ColorForth and MachineForth prefer to take a bit more functional approach than ANSI Forth does. This change in the design of the Forth environment is due to ChuckMoore's realization that it maps better onto the generated binary image, and is thus closer to the machine.

However, there is more than one way to skin a cat. One technique that I use a lot in Forth to eliminate the arrow pattern is to use structured returns -- since Forth exposes the return address stack, I can do stuff like this: 

  : isItA?             dup anA? if r> drop handleConditionA then ;
  : isItB?             dup aB?  if r> drop handleConditionB then ;
  : isItC?             dup aC?  if r> drop handleConditionC then ;
  : defaultAction      neitherA norB norC soDoSomethingUsefulHere ;


  : makeDecisionHere   isItA?  isItB?  isItC?  defaultAction ;
  : foo                ... makeDecisionHere ... ;
-- SamuelFalvo?

Proposed Solution: Use "if (condition()) { ifBlock(); } if (!condition()) { elseBlock(); }" -- assuming that ifBlock() can't possibly change the result of condition().

Problem: Writing the code this way generally implies to the reader that both "if" statements could evaluate to True. That is, that the ifBlock() could change conditions so that "!condition()" is true. Since this is normally not the case, one wouldn't want to write code that suggests it *is* the case.

Bad solution. I've seen it in code from time to time, and it's always bad. Consider... 

  if x and y and not z then
...  Do this.  ...
  endif
  if not x or not y or z then
...  Do some other stuff.  ...
  endif
Whenever I see this, I check to make sure that no side-effects change x, y or z in the first block, and then I change the second "if" to an "else." -- JeffGrigg

Does someone really think this is better than having an 'else'? Not only is it confusing, but it's a maintenance problem waiting to happen.

No, I'm going to say this [use of if x .. endif ; if not x .. endif instead of else] is absolutely wrong. It violates OnceAndOnlyOnce for starters, plus it increases the number of functional points, and the number of fixes (i.e. things to read and understand). -- SunirShah

Thanks for the example. It reveals the smell by showing what the reader has to "expand" when reaching the "else". Hit me, but I'm not even sure, whether the above inversion in correct (damned operator precedence). --DierkKoenig

No, it's not an issue of "else" being smelly, the problem is the complex condition in the if statement: Use DecomposeConditional to change this 
  if x and y and not z then
...  Do this.  ...
  else
...  Do some other stuff.  ...
  endif
to 
  if customerGetsADiscount() then
...  Do this.  ...
  else
...  Do some other stuff.  ...
  endif
then the else clause is really easy for the reader to understand.

Now, in some cases, the "ifBlock" might have "side effects" that change the "condition()":

This [checking for side effects] is absolutely essential. The proposed construct is very like a construct which I have often seen and even written: 

  if line_disconnected then
try to recover line
  endif


  if not line_disconnected
send data on line
  endif
(usually I would have an else cause on the second if to deal with the case when the line can't be recovered.) In this case changing the code to an if else would not allow for the recover line operation changing the line status. -- ChrisBrooking?

If you are that worried that people might forget what is going on, why not just put a comment? 

 if (condition())
 {
ifBlock();
 }
 else /* condition() */
 {
elseBlock();
 }

[Comment on "else" in previous example]

For me that is a valid solution, although it violates the OnceAndOnlyOnce rule in an unusual way and adds the 'Comment' smell. When refactoring you sometimes want to trade one smell for the other. --DierkKoenig

[Comment on "else" in previous example] is a bad idea because it violates OnceAndOnlyOnce. It is a good idea if the second condition isn't the negation of the first. I do this when optimizing for code space (but not normally because it is pathological). For instance: 

 // 'size'
 if( 's' == *argument || 'S' == *argument ) {
...
 } 
 // 'name'
 else // if( 'n' == *argument || 'N == *argument ) { 
...
 }
but the assumption here is that there are only 'size' and 'name'. If I was making a robust system, I would probably do something more like 

 if( !strcmpi("size", argument) ) {
...
 }
 else if( !strcmpi("name", argument) ) {
...
 }
 else {
// ERROR
 }
and then I'd get fed up and use the CollectionAndLoopVsSelectionIdiom with function pointers: 

 #define NUMELEM(x) (sizeof (x) / sizeof *(x))


 bool size();
 bool name();


 bool parseArgument( char const *szArgument )
 {
static struct {
  char const *szArgument;
  bool (*pfnParser)();
} aParsers[] = {
  "size",size,
  "name",name,
};


for( int i = NUMELEM(aParsers); i--; ) 
 if( !strcmpi(aParsers[i].szArgument, szArgument) )
return aParsers[i].pfnParser();
return false;
 }
And then I'd kill the authors of GCC for screwing up the CollectionAndLoopVsSelectionIdiom. (Sorry, it's been a bad morning. @#$%!@# 2.9.7.) -- SunirShah

I've been known to do... 

  if ( 's' == *argument || 'S' == *argument )// 'size'
  {
...
  }
  else
  {
assert( 'n' == *argument || 'N == *argument );// 'name'
...
  }
I've also been known to put a "negated" condition as a comment on the else clause, but the need to do this is a really strong indication that the method as exceeded all rational size limitations. ;-> -- JeffGrigg

Discussion:

It is not the "else" that is smelly itself but it easily leads to smelly constructions (same argument like with temporary variables, cf. ReplaceTempWithQuery). --DierkKoenig

Think also how "good" a GOTO (MultipleInheritance?, Inflection?, MultiMethods?, etc.) can be. It can be very clear and elegant when used wisely. But still, we don't use it if we can easily avoid it.

Writing rules in an expert system that didn't have user-defined ordering of the rules quickly convinced me of the value of "else" and "otherwise" when writing business logic. After writing rules for "screen posted successfully", "common error #1", "common error #2" and "common error #3", I then had to write a rule to catch the "anything else" case. Without some kind of "otherwise" syntax, it's really hard to ensure that your rules always cover all possible states, and have no overlap. -- JeffGrigg

Could you show a real hairy if-else example that you would resolve in some other way (even by using a different programming paradigm, like logical programming)? This "DarkPattern" has a smell of its own.

While if can be a ticklish structure, there isn't really a way to completely avoid it. When a series of values is to be used to "decide" what to do next, then a switch() { case: } construct can work, or a vector table, or some similar case concept.

Where it gets a bit dirty is when the else side of the structure doesn't evaluate the same elements. One of my favorite examples of this is the "bed of snakes" code that we used to implement a parser that separated names into their component parts (prefix, first, middle, last, suffix) when one or more parts could be missing.

The essence of this problem is that the if clause has eliminated one whole category of checks, so the else clause(s) must now inspect the remaining possibilities.

The only language I've used that allowed a flexible case type of structure was FoxPro, where each case was evaluated separately (as though it were an elseif).

In CeeLanguage, we had to get more creative to make the logic clear as well as effective. It was still a BedOfSnakes?.

It was, however, an interesting excursion into a kind of PredicateLogic applied to name structure.

-- GarryHamilton

What's the difference between a Foxpro DO CASE versus a series of IF-ELSEIF in other languages? And what is this BedOfSnakes?...do you just mean messy code?

I've been thinking about how this relates to long (cond) clauses. (cond)'s are seldom nested at all; in the "classical" style, there's one cond per function. They're practically long if-else if-else if-else constructs. I don't find them hard to read, usually I don't even want to know the condition of the "else" part unless I've also seen what the other parts do. Maybe it's the same thing as the ColorForth idiom above. Basically, if you have one cond per function, you don't need to track various execution paths, just make sure that every (cond) clause obeys the contract of the function.

Because resource allocation and deallocation doesn't really fit this scheme, it's not done via (cond) or (if) in lisp-like languages, but rather via function wrappers.

"Else Considered Smelly"

Seriously? Some people seem to try very hard to find alleged smells. SmellingTooManySmellsIsaSmell?. You should get your nose checked out. -- AdrianWillenbuecher?

In the chemical industry, people who work with strong smells can find that their nose is desensitised even to the point of being unable to detect the smell at all. I wonder if that is a risk here as well. -- JohnFletcher

I agree. However, your analogy doesn't apply here. Not the ones that smell it are the vast majority (as would be the case in your analogy), but those that don't smell it are. And, as has been shown above, the alleged smells either don't originate from the else itself, or the "solutions" are by far worse then the "problem".

Coming up next: FunctionsConsideredSmelly?. Long, unfactored code, undescriptive names, missing comments, recursion and non-termination: the use of functions is often a sign for bad code that calls for improvement. Tune in at 10pm to see how gotos and global variables can help you get rid of this stench. -- AdrianWillenbuecher?

CategoryConditionalsAndDispatching

EditText of this page (last edited February 11, 2010) or FindPage with title or text search