Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I understand that as far as the compiler is concerned, the following lines are equivalent.

if (aPtr) {...}
if (aPtr != NULL) {...}
if (NULL != aPtr) {...}

However, I find the second form a lot more readable than the first form.

The third form has saved me from debugging problems a lot more times than I care to remember. How so?

It's very easy to make the mistake of writing

if (aPtr = NULL) {...}

when you meant to write

if (aPtr != NULL) {...}

You can imagine the bugs arising from that typing error. This error is easily avoided using the third form. The statement

if (NULL = aPtr) {...}

will result in a compiler error.

As a matter of habit, I have adopted writing if (NULL != aPtr) and if (NULL == aPtr) for pointer comparisons.

I am interested in finding out what others do for such use cases.

EDIT

Is having a function/macro is_null a good compromise between if (aPtr) and if (aPtr != NULL)?

inline bool is_null(void* p) {return (p == NULL);} /* For C++ */
#define is_null(p) ((p) == NULL) /* For C */
share|improve this question

closed as off-topic by 200_success Mar 29 at 1:44

  • This question does not appear to be a code review request within the scope defined in the help center.
If this question can be reworded to fit the rules in the help center, please edit the question.

7  
FYI: The name for your preferred style is "Yoda conditions" –  Flambino Mar 28 at 20:07
    
@Flambino Thanks for the link. Didn't know there was a name for it :) –  R Sahu Mar 28 at 20:17
1  
The third form is terrible. It does not save you any more than turn on the correct compiler switches (in fact the correct compiler switches will help you a lot more). –  Loki Astari Mar 28 at 22:23
    
2  
While this is an interesting question, this question is currently off topic for Code Review because we require actual code, not hypothetical skeleton code. Could you supply code so that there is sufficient context for custom advice? Also, c and c++ differ subtly, so please tag for one language or the other. –  200_success Mar 29 at 0:13

3 Answers 3

up vote 10 down vote accepted

The three different styles aim for different goals. Those goals (respectively) are:

  1. Brevity
  2. Readability
  3. Security

If you are working on an existing code base, then you should follow the already existing conventions. Otherwise I would go with readability. Readable code is easiest to maintain in the long run.

Example 1: Readable code is less ambiguous

Let's say you have a program where you are using error codes rather than exceptions. This could be embedded code or you could be working with some sort of low-level API. Let's also say that either somebody else wrote this code or you wrote this a long time ago:

if (!Func1()) {
    return false ;
}

if (!Func2()) {
    return false ;
}

if (Func3()) {
    return false ;
}

if (!Func4()) {
    return false ;
}

Look at the third if-statement. Even noticing this difference can be difficult if you're going through many lines of code.

if (Func3()) {
    return false ;
}

Is this a bug? Did the original programmer mean this or were they coding rapidly and they actually meant !Func3()? Now you have to step-through Func3() to figure this out.

Let's look at a more explicit version:

if (Func1 () == false) {
    return false ;
}

if (Func2 () == false) {
    return false ;
}

if (Func3 () == true) {
    return false ;
}

if (Func4 () == false) {
    return false ;
}

It's now much easier to spot the different function. It is also less likely that the programmer who wrote this accidentally typed true when they meant false.

Example 2: Readable code is easier to understand

if ((pOne == NULL || pTwo != NULL) && SomeCondition () == false) {
}

if ((NULL == pOne || NULL != pTwo) && false == SomeCondition ()) {
}

I can read the first if-statement from left-to-right in one pass and understand what's going on. When I read the second if-statement, I find myself "backtracking" to understand all of the logic. In other words, it would take me longer to parse and debug code that is written the second way.

I do lose some nice free security by writing readable code, but I think it's an acceptable sacrifice in this particular case.

share|improve this answer
1  
The problem with if (!condition) has more to do with the overuse of ! than the brevity, IMO. Were those conditions positive, or if it were like if (Func1() && Func2() && Func3() && Func4()) { /* magic happens*/ } else return false;, it'd be easier to read. :P –  cHao Mar 28 at 22:24
1  
I totally agree with your example 2, but I have to add that normally methods aren't named Func3 and should make it more obvious that there's a ! there. What the method does also matters, perhaps reverse the return of the method to make it match the pattern, or create an additional method which simply reverses the result. –  Simon André Forsberg Mar 28 at 22:25

As a matter of habit, I have adopted writing if (NULL != aPtr) and if (NULL == aPtr) for pointer comparisons.

Well get out of the habit.
Its horrible to read and thus maintain. And worst of all gives a false sense of security. If you want to get a compile time error when this happens accidentally then turn your warning level up and treat all warnings as errors.

A warning by the C++ compiler is an indication of a logical error in your code. Your code should compile with zero warnings. So set your compiler to treat all warnings as errors and then turn on as many warnings as you can put up with:

My basic set of warnings. (This is my absolute minimum I usually try and add more).

-Werror -Wall -Wextra -Wstrict-aliasing -Wunreachable-code -ansi -pedantic 
// There are equivalent switches on dev studio


ghp.cpp:5:14: error: using the result of an assignment as a condition without
      parentheses [-Werror,-Wparentheses]
    if (aPtr = NULL)
        ~~~~~^~~~~~
ghp.cpp:5:14: note: place parentheses around the assignment to silence this
      warning
    if (aPtr = NULL)
             ^
        (          )
ghp.cpp:5:14: note: use '==' to turn this assignment into an equality comparison
    if (aPtr = NULL)
             ^
             ==
1 error generated.
share|improve this answer

Ideally, a piece of code should communicate the intention with the reader clearly and unambigiously while having minimal cognitive overhead. You should take decisions about your coding style while taking that into account.

About this specific problem; in normal circumstances I would go for the first option. It is a well known idiom that every primitive types and their pointers evaluate to false if and only if their numerical value is zero. Knowing this, explicitly checking if some variable, in fact, is zero is pointless.

That being said, as jliv902 demonstrates being verbose absolutely has its place. In the end, it kind of depends on the context.

share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.