Search code examples
cembeddedmicrochipmplab

Any reason for if(function() == TRUE) in C


The Question: Does doing if(SomeFunction() == TRUE) instead of doing if(SomeFunction()) protect against some type of coding error? I'm trying to understand if this is protecting from some hidden land-mine, or if it's the result of someone writing code who didn't quite understand how expressions are evaluated. I understand that if done right, both of these things evaluate the same. Just like if(value == 42) and if(42 == value) evaluate the same - still, some prefer the 2nd version because it produces a compiler error if someone typo's the == and writes = instead.

Background: I've inherited some embedded software that was written 4 or 5 years ago by people who don't work here anymore. I'm in the middle of some refactoring to get rid of multi-hundred line functions and global variables and all that jazz, so this thing is readable and we can maintain it going forward. The code is c for a pic microprocessor. This may or may not be relevant. The code has all sorts of weird stuff in it that screams "didn't know what they were doing" but there's a particular pattern (anti-pattern?) in here that I'm trying to understand whether or not there's a good reason for

The Pattern: There are a lot of if statements in here that take the form

if(SomeFunction() == TRUE){
  . . .
}

Where SomeFunction() is defined as

BOOLEAN SomeFunction(void){
  . . .
  if(value == 3)
    return(FALSE);
  else
    return(TRUE);
}

Let's ignore the weird way that SomeFunction returns TRUE or FALSE from the body of an if statement, and the weird way that they made 'return' look like a function invocation.

It seems like this breaks the normal values that c considers 'true' and 'false' Like, they really want to make sure the value returned is equal to whatever is defined as TRUE. It's almost like they're making three states - TRUE, FALSE, and 'something else' And they don't want the 'if' statement to be taken if 'something else' is returned.

My gut feeling is that this is a weird anti-pattern but I want to give these guys the benefit of the doubt. For example I recognize that if(31 == variable) looks a little strange but it's written that way so if you typo the == you don't accidently assign 31 to variable. Were the guys that wrote this protecting against a similar problem, or is this just nonsense.

Additional Info

  • When I wrote this question, I was under the impression that stdbool was not available, but I see now that it's provided by the IDE, just not used in this project. This tilts me more towards "No good reason for doing this."
  • It looks like BOOLEAN is defined as typedef enum _BOOLEAN { FALSE = 0, TRUE } BOOLEAN;
  • The development environment in question here is MPLAB 8.6

Solution

  • Is there a good reason for doing if(SomeFunction() == true) instead of doing if(SomeFunction())

    No.

    If SomeFunction() returns a result of type _Bool, then the equality comparison should be reliable (assuming that the evaluation of the result did not involve undefined behavior). But the use of TRUE rather than true, and the type name BOOLEAN rather than _Bool or bool, suggest that the result is not an actual _Bool (available only in C99 or later), but some ad-hoc Boolean-like type -- perhaps an alias for int.

    A value of any scalar type can be used as a condition in an if statement. If the value is equal to zero, the condition is false; otherwise, the condition is true. If TRUE is defined as 1, and SomeFunction() returns, say, 3, then the test will fail.

    Writing

    if (SomeFunction()) { /* ... */ }
    

    is simpler, clearer, and more likely to behave correctly.

    Note, for example, that the isdigit() et al functions declared in <ctype.h> do not just return 0 or 1; if the argument is a digit, isdigit() can (and does) return any non-zero value. Code that uses it is expected to handle it correctly -- by not comparing it for equality to 1, to true, or to TRUE.

    Having said that, there might be a valid reason to compare something for equality to TRUE -- if it matters whether the result is equal to TRUE or has some other non-zero value. But in that case, using the names BOOLEAN and TRUE is misleading. The whole point of a Boolean type is that values are either true or false; there's no "maybe", and if there happen to be different representations of truth, you shouldn't care which one you have.

    The guideline I try to follow is:

    Never compare a logically Boolean value for equality or inequality to true or false (or 0, 1, FALSE, TRUE). Just test the value directly, with a ! operator if you want to invert the test. (A "logically Boolean" value either is of type _Bool, or is intended to distinguish between truth and falsehood with no additional information. The latter can be necessary if _Bool is not available.) Comparison to false can be safe, but there's no reason to do it; comparing the value directly is still clearer.

    And if someone tells you that

    if (SomeFunction() == true)
    

    is better than

    if (SomeFunction())
    

    just ask them why

    if ((SomeFunction() == true) == true)
    

    isn't even better.

    See also section 9 of the comp.lang.c FAQ. Its emphasis on pre-C99 solutions is perhaps a bit dated, but it's still valid.

    UPDATE: The question asks about a function that returns the value TRUE of type BOOLEAN, defined something like this:

    typedef enum { FALSE, TRUE } BOOLEAN;
    

    Such definitions were useful in pre-1999 C, but C99 added the predefined Boolean type _Bool and the header <stdbool.h>, which defines macros bool, false, and true. My current advice: Use <stdbool.h> unless there's a serious concern that your code might need to be used with an implementation that doesn't support it. If that's a concern, you can use

    typedef enum { false, true } bool;
    

    or

    typedef int bool;
    #define false 0
    #define true 1
    

    (I prefer the first.) This isn't 100% compatible with the C99 definitions, but it will work correctly if you use it sensibly.