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

Question

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

Answer 1

Since in C any non-zero value is considered true and only zero false you should never compare to one specific TRUE macro definition in any event. It is unnecessarily specific. The form:

if( fn() )

is the simplest form, but if you do prefer to compare to a specific value, then only compare to FALSE thus:

if( fn() != FALSE )  // Safer than '== TRUE', but entirely unnecessary

which will work for all reasonable definitions of FALSE and also if fn() is not BOOLEAN. But it remains totally unnecessary.

Personally for easier debugging I'd prefer:

 BOOLEAN x = fn() ;
 if( x )

As well as being able to observe the return value in your debugger before entering or skipping the conditional block, you have the opportunity to name x something self documenting and specific to the context, which the function name might not reflect. In maintenance you are more likely to maintain a variable name than correct a comment (or many comments). In addition x is then available to use elsewhere rather then calling fn() multiple times (which if it has side effects or state may not return the same value).

Another problem with user defined boolean types and values is that the definitions may not be consistent throughout - especially if you use third-party code whose authors also thought it a good idea to define their own using the same symbol names as yours. If the names differ (such as BOOL, BOOLEAN or OS_BOOL for example), when your code interfaces to this third-party code, you then have to decide whose boolean type should be used in any particular circumstance, and the names of TRUE and FALSE are likely to clash with redefinition warnings or errors.

A better approach would be to update the code to use stdbool.h and the real boolean type bool (an alias for the built in _Bool in C99) which can have only two values true and false. This will still not protect you from the case where fn() is not a bool function and returns an integer other then zero or one, but there is then the chance that the compiler will issue a type mismatch warning. One of the best things you can do to refactor legacy code in and case is to set the warning level high and investigate and fix all the warnings (and not just by liberal casting!).