Refactor Negate If

It's easy to reverse the sense of an if/then/else. This is typically done to eliminate "double-negative" logic:

From...

  If Not GetTheStuff() Then
' Do without the stuff.
  Else
' Do the work that we do when we are >not unable< to get the stuff.
' (i.e.: We >have< the stuff!!!)
  End If

to...

  If GetTheStuff() Then
' Work >with< the stuff.
  Else
' Do >without< the stuff.
  End If

This can be used to create or eliminate empty "then" blocks:

From...

  If Condition() Then
' Do stuff (that's about to be deleted).
  Else
' Do other stuff.
  End If

to...

  If Not Condition() Then
' Do other stuff.
  End If

From...

  If Not Condition() Then
WorkDoneHere()
  End If

to...

  If Condition() Then
' Now we have a place to add the new work (without using double negative logic).
  Else
WorkDoneHere()
  End If

After refactoring (eliminating code from the "then" block), I often find myself changing code...

from...

  If Condition() Then
  Else
' Do this stuff.
  End If

to...

  If Not Condition() Then
' Do this stuff.
  End If

That's why I hesitate to do this. I would prefer the first alternative; it avoids negative logic. And one day, something will have to be done when Condition() is true...

When that happens, change it back. Meanwhile, YouArentGonnaNeedIt.

Why change it around in the first place? It's only simpler (less text) because some programming languages happen to have the rather arbitrary rule that you can omit the else-clause, but you cannot omit the then-clause.

If you used PerlLanguage, you could use unless to include the negative in the statement:

  unless (condition()) {
do_something();
  }

  if (GetDataWithPrimaryAlgorithm())
// Way cool; it worked!
; // (Take no corrective action.)
  else
GetDataUsingBackupAlgorithm();

  LotsOfCodeHereUsingTheData();

Empty "then" blocks are syntactically valid (or can be faked) in every block-structured language I've seen. But they're clunky. Isn't it more clear to say "if getting the data the normal way failed, then try the alternate way?" The code above says "if getting the data the normal way was successful, then fine, don't do anything in particular about it; but if it failed, try the alternate way."

Now, I admit that later, often I find myself refactoring

  if ! GetData()
return 0;
  iResult = LotsOfInlineCodeHere();
  return iResult;

into

  if GetData()
iResult = LotsOfInlineCodeHere();
  else
iResult = 0;
  return iResult;

  iResult = 0;
  if GetData()
iResult = LotsOfInlineCodeHere();
  return iResult;

But this doesn't bother me, because I know how to program, and I have good tests. -- JeffGrigg

  return (GetData() ? LotsOfInlineCodeHere() : 0);

I prefer this form. --Sebastian Davids

  if GetData()
return LotsOfInlineCodeHere();
  return 0;

I might do

  if GetData()
return LotsOfInlineCodeHere();
  else
return 0;

But otherwise, I concur. -- JeffGrigg

I will refactor If statements of this kind regardless of the presence of Negatives - I find it best to move the shortest case first. E.g.:

  If X then
// 15 lines of code
  else
// 2 lines of code
  end if

Is quicker to read, when the simpler case is moved first

  If Not X then
// 2 lines of code
  else
// 15 lines of code
  end if

This works for me because that 15 lines of code is enough to fill up my short term memory and make it harder to parse the code. But a simple logical statement doesn't fill up my mind - even if it has a negative in front of it. If the negatives are six or seven deep - then you'd definitely have to refactor though - the brain would too often lose track

-- LeonBambrick

The length of a block shouldn't determine it's order. Long blocks are smelly and should be refactored with ExtractMethod. Negative if statements are smelly, too. -- EricHodges

This is just plain incorrect. Just because something is 15 lines long is not necessarily a call to refactor(that's why it's called a CodeSmell and not a CodeSin), yet Leon is correct that readability increases if the longer block comes second in the if-else.

Negative if statements are indeed a CodeSmell, but it comes down to a judgment call depending on circumstances.

Just a few short years ago I amazed a junior engineer by rewriting a chunk of his code that had a few hard-to-find bugs. Originally it looked like:

  func() {
if (cond1) {
f1();
if (cond2) {
f2();
if (cond3) {
  f3();
  if (cond4) {
f4();
if (cond5) {
do_the_main_point_at_last();
}}}}}
  }

I changed this to:

  func() {
if (!cond1) return;
f1();
if (!cond2) return;
f2();
if (!cond3) return;
f3();
if (!cond4) return;
f4();
if (!cond5) return;
do_the_main_point_at_last();
  }

His code was nested enough to wrap way around the edge of the window. His conditions were more complex than I've indicated; some were positive, some were negative, but all acted as guards against the innermost code. By eliminating the nesting, I made it vastly more readable, and the bugs went away automatically, too, due to lessening the complexity and making it obvious which were the correct guard conditions.

The fact that I introduced some negative if statements along the way was not a bad thing in this case. Nor was there a reason to refactor into multiple functions; the whole thing was a single concept: Do one thing after some sequential setup which required sequential guards.

Never say never! Readability depends on the circumstances; there is no law of programming that doesn't have exceptions. One must always think about what is best in the situation at hand; relying on rules to do your thinking for you is reprehensible intellectual laziness, and insisting that others do the same is worse.

-- DougMerritt

I call this "filtering" and much prefer it to endlessly nesting if statements. It is much more readable.

The problem with the empty then-branch becomes more complicated (to read and understand), if you have nested conditions. At least in this case its really worth it to refactor. -- Thorsten van Ellen

I would very hesitant to call this type of change "refactoring." If this is the level change one needs to make to a program, then the program is in pretty good shape. I would fear that this level of change would not lead to code improvement, but would lead to a coding preference battle with the code being changed back and forth by different parties. The latter is just going to increase the chance that the code gets mis-factored sometime leading to a failure.

First off, you seem to have missed my example above, where it clearly was refactoring, where it clearly was not in "pretty good shape" (lines wrapped due to overly deep nesting, multiple bugs hard to find), and unquestionably did improve the code. Secondly, to dismiss these things as "coding preference battles" utterly misses the point. I don't screw with other people's code when it is already more or less readable and working, whereas in this case, if the guy had refused to accept my changes, I would have fired him on the spot; we had enough problems.

However you should find it telling that I said he was "amazed", and furthermore I meant "he was amazed at how much it improved his code, and asked me what the trick was that I used to pull that off".

Think that anything goes, that it's all just personal opinion, that there is no standard for right and wrong, is a philosophy that is fading even in the humanities, but it never did work in the slightest in the hard sciences and technology. That just leads to bad code because you're afraid to disagree with anyone.

I might suggest a more careful look at the specific example with the multiple cascaded if statements. I would contend both alternatives shown exhibit the same level of fragility, due to the sequential nature of the evaluation. Inserting an intermediate evaluation would be risky. Some alternatives that would improve the structure and reduce the fragile nature of the code would be:

Replace the if statements with polymorphism, although the if structure should still be improved if a class factory is also used.
Push the if statement into its associated function, ala a guard clause, and then cascade the functions. This would simplify the if structure by breaking it apart, but will create a deep function call chain.
Complete each conditional test so that it stands alone and explicitly call each method for each unique condition (shown below). This emphasizes the branching structure, but hides the sequential nature of the lower level functions.

  func() 
  {
if (cond1 * !cond2)
{
f1();
}
else if (cond1 * cond2 * !cond3)
{
f12();
}
else if (cond1 * cond2 * cond3 * !cond4)
{
f123();
}
  }

  f123()
  {
f1();
f2();
f3();
  }

Complete the conditional statements as before and push them into the associated functions. This emphasizes the sequential nature of the lower level functions.

  func()
  {
f1(cond1, cond2);
f2(cond1, cond2, cond3);
f3(cond1, cond2, cond3, cond4);
  }

There are a lot of ways to actually Refactor this code that actually improve the structure. Rewriting the if statement sense really does not change the structure and, at least in my experience, tends to lead to coding style preference battles that I really would rather avoid.

Disclaimer, I write this to show another notation, to because this is best, worst, or even unique.

I find myself wanting to use CallWithCurrentContinuation.

In a java'y style syntax:

  func(){
withCurrentContinuation(escape){
f1(cond1, escape);
f2(cond2, escape);
f3(cond3, escape);
f4(cond4, escape);  
whatWereTryingToDo();
}
  }

With the fn() functions testing the condition, and accepting a method to be called if they fail. Unless they were being reused for another purpose (which would seem likely to be questionable anyway), I'd just embed the condition in the function and have them simply accept an escape.

Most of the examples on this page would likely compile to the same binary... this version to my mind best describes what's happening. Of course, if func() does nothing else, then a 'return;' statement is effectivily exactly this code, assuming that the 'escape' method isn't returned by func().

-- WilliamUnderwood (not really trying to be smug and schemey, but just can't help it)

The problem with your junior programmer was likely just the opposite: students are taught that there is One Right Way, and that way should never be tweaked. In this case, many programming courses teach an inviolable rule that There Shall Be One Exit Point For A Function - leading to monstrosities like the one you cited. I've been out of school for quite a while, but last year I helped a student with a C++ assignment; she lost points on the assignment because she took my advice and changed some code from an if/if/if/if model to an early-exit model, violating the One Exit Point law.

Very sad. I had a similar experience back when Structured Programming was the new hot thing, and I used a single fatal-error exit GOTO out of deep nesting, along with a 50 line comment explaining why, and referencing Wirth's text (used for the class) where Wirth himself used a goto for the same reason in the same circumstances. The grader took 10% off, ignoring the comment. I was incensed and went to the prof, who merely said "of course" and gave me full credit. He mumbled something about that it's not the grader's job to think, that's the prof's job. :-)

Never settle for an unfair grade if you can help it.

The Single Entry/Single Exit condition is part of the definition of Structured Programming, since the proof that any flow graph could be rewritten in a structured way depends on it, so it makes exactly as much sense to avoid it as it does to avoid GOTOs: almost always -- and especially in class.

There are some tricks you can do with booleans.

 func() {
boolean do_it = true;
if (!cond1) do_it = false;
if (do_it) f1();
if (!cond2) do_it = false;
if (do_it) f2();
if (!cond3) do_it = false;
if (do_it) f3();
if (!cond4) do_it = false;
if (do_it) f4();
if (!cond5) do_it = false;
if (do_it) do_the_main_point_at_last();
 }

This is not a valid refactoring, because you are always executing cond2 .. cond5, which might have side effects.

Would this be more to your liking? Conditions are only executed if the prior condition passed:

  func() {
boolean do_it = true;

if (cond1) f1( );
else do_it = false;

if (do_it && cond2) f2( );
else do_it = false;

if (do_it && cond3) f3( );
else do_it = false;

if (do_it && cond4) f4( );
else do_it = false;

if (do_it && cond5) do_the_main_point_at_last( );
  }

WyattMatthews

In StructuredProgramming with GOTOs DonKnuth pointed out that some uses of goto are equivalent to storing booleans in the program counter.

Personally, I consider "if not <condition>" with an else clause to be a CodeSmell: The answer to the question, "When is the else clause executed?" is a double negative. -- JeffGrigg

( EditHint: move discussion of internal exists to InternalLoopExitsAreOk )

[CategoryRefactoring/RefactoringLanguage]