56

I read some legacy code:

if ( 1 || !Foo() )

Is there any seen reason why not to write:

if ( !Foo() )
LihO
  • 41,190
  • 11
  • 99
  • 167
Elad Benda
  • 35,076
  • 87
  • 265
  • 471
  • 27
    '1 || something()' will always yield true. So the two statements are not equivalent, but as to why this is done, pass. – SinisterMJ Oct 10 '13 at 09:43
  • 10
    `if (1 || !foo())` <==> `if (1)`. – devnull Oct 10 '13 at 09:45
  • Why not just to remove the conditional? You say that `1 || foo()` == `foo()`. This means that the code behind them is useless. You may remove it altogether with the conditions. – Val Oct 10 '13 at 11:57
  • 27
    Most probably this was done temporarily to force the conditional to true (maybe a bug, workaround or refactoring) and not put back. Use the annotate/blame feature of your source control to find the version where this changed -- you might get a useful comment or bug database entry to help work out why it was changed – the_mandrill Oct 10 '13 at 13:52
  • 2
    Maybe Foo() is implemented as `return rand() % 2;` – Happy Green Kid Naps Oct 10 '13 at 16:27
  • 1
    `1 && !Foo()` is equal to `!Foo()` – Niklas R Oct 12 '13 at 09:10

4 Answers4

134

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.

Luchian Grigore
  • 253,575
  • 64
  • 457
  • 625
  • 23
    Sounds like something that what inserted during development/debugging, and left there either because someone forgot to remove it, or by fear of breaking something. – Medinoc Oct 10 '13 at 09:45
  • In any case this is a better solution than commenting out because the compiler keeps checking the inactive code. I do this all the time for debugging reasons. – usr Nov 24 '13 at 17:33
  • only really really experienced developer know this. +1 – Baby Mar 27 '14 at 09:23
44

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

Maroun
  • 94,125
  • 30
  • 188
  • 241
  • 6
    So `if(1 || !Foo() ) {code}` == `if(1){code}` == `{code}` while `if (!Foo())` can be either `{code}` or just `;` depends on value returned by `Foo()`. :) – Grijesh Chauhan Oct 10 '13 at 09:56
  • I extend you last expressions.. EDIT: for debugging purpose either use a macro `DEGUG_ON = 1` or comment it. `if (DEGUG_ON || !Foo()) ` (in fact I will comment code). ;) – Grijesh Chauhan Oct 10 '13 at 14:52
  • *"It's highly recommended to have a macro for this purpose, say DEBUG_ON 1"*... yes except code readability is your last concern when you're trying to find that damn bug :-) – user541686 Oct 16 '13 at 08:47
  • 1
    @Mehrdad Indeed. But this bug check could turn into a bug itself.. Or mae a confusion, like it made to OP :D – Maroun Oct 16 '13 at 08:49
11
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)

LihO
  • 41,190
  • 11
  • 99
  • 167
  • *`you don't want to wait till that condition will evaluate to true`* yes basically short-circuits is a technique to save executions! – Grijesh Chauhan Oct 10 '13 at 10:06
  • @GrijeshChauhan: I was not referring to performance, neither time of execution. I was referring to specific criteria being met. – LihO Oct 10 '13 at 10:13
  • ouch that's a strange language feature. I was believing that conding was always evaluated if it had side effects (so basically I was believeing that non-const methods and static functions was evaluated, but I tried in a simple "hello world" and was never able to get condition evaluated in the first case O_O – CoffeDeveloper Oct 12 '13 at 08:59
10

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.

PatrickV
  • 2,057
  • 23
  • 30