Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Is there any reason for using if(1 || !Foo())?

I read some legacy code:

if ( 1 || !Foo() )

Is there any seen reason why not to write:

if ( !Foo() )
like image 531
Elad Benda Avatar asked Oct 10 '13 09:10

Elad Benda


4 Answers

if (1 || !Foo() ) will be always satisfied. !Foo() will not even be reached because of short-circuits evaluation.

This happens when you want to make sure that the code below the if will be executed, but you don't want to remove the real condition in it, probably for debug purposes.

Additional information that might help you:

  • if(a && b) - if a is false, b won't be checked.
  • if(a && b) - if a is true, b will be checked, because if it's false, the expression will be false.
  • if(a || b) - if a is true, b won't be checked, because this is true anyway.
  • if(a || b) - if a is false, b will be checked, because if b is true then it'll be true.

It's highly recommended to have a macro for this purpose, say DEBUG_ON 1, that will make it easier to understand what the programmer means, and not to have magic numbers in the code (Thanks @grigeshchauhan).

like image 42
Maroun Avatar answered Nov 04 '22 01:11

Maroun


The two are not the same. The first will never evaluate Foo() because the 1 short-circuits the ||.

Why it's done - probably someone wanted to force entry in the then branch for debugging purposes and left it there. It could also be that this was written before source control, so they didn't want the code to be lost, rather just bypassed for now.

like image 200
Luchian Grigore Avatar answered Nov 04 '22 02:11

Luchian Grigore


1 || condition

is always true, regardless whether the condition is true or not. In this case, the condition is never even being evaluated. The following code:

int c = 5;
if (1 || c++){}
printf("%d", c);

outputs 5 since c is never incremented, however if you changed 1 to 0, the c++ would be actually called, making the output 6.


A usual practical usage of this is in the situation when you want to test some piece of code that is being invoked when the condition that evaluates to true only seldom is met:

if (1 || condition ) {
    // code I want to test
}

This way condition will never be evaluated and therefore // code I want to test always invoked. However it is definitely not the same as:

if (condition) { ...

which is a statement where condition will actually be evaluated (and in your case Foo will be called)

like image 11
LihO Avatar answered Nov 04 '22 00:11

LihO


The question was answered properly - the difference is the right side of the or operation is short-circuited, suggesting this is debug code to force entry into the if block.

But in the interest of best practices, at least my rough stab at a best practice, I'd suggest alternatives, in order of increasing preference (best is last):

note: noticed after I coded examples this was a C++ question, examples are C#. Hopefully you can translate. If anyone needs me to, just post a comment.

In-line comment:

if (1 /*condition*/) //temporary debug

Out-of-line comment:

//if(condition)
if(true) //temporary debug

Name-Indicative Function

//in some general-use container
bool ForceConditionForDebug(bool forcedResult, string IgnoredResult)
{
      #if DEBUG
          Debug.WriteLine(
              string.Format(
                  "Conditional {0} forced to {1} for debug purposes",
                  IgnoredResult,
                  forcedResult));
          return forcedResult;
      #else
          #if ALLOW_DEBUG_CODE_IN_RELEASE
              return forcedResult;
          #else
              throw new ApplicationException("Debug code detected in release mode");
          #endif
      #endif
}

//Where used
if(ForceConditionForDebug(true, "condition"))...

//Our case
if(ForceConditionForDebug(true, "!Foo()"))...

And if you wanted a really robust solution, you could add a repository rule to source control to reject any checked in code that called ForceConditionForDebug. This code should never have been written that way because it obviously doesn't communicate intent. It never should have been checked in (or have been allowed to be checked in) (source control? peer review?) And it should definitely never be allowed to execute in production in its current form.

like image 10
PatrickV Avatar answered Nov 04 '22 01:11

PatrickV