Approxion

If you have been coding in C and C++ for some time, you know that it is easy to introduce bugs: type in a single wrong character and you wreak havoc. It is therefore a good idea to avoid using certain (dangerous) language features or at least using them in a way that is safer.

My favorite example is always putting braces around blocks, even if the block only consists of a single statement:

if (retries <= MAX_RETRIES) {
    enableOutput();
}

I've witnessed cases where really experienced programmers omitted these two "redundant" characters for brevity only to be later faced with this classic bug:

if (retries <= MAX_RETRIES)  // Added today.
    enableOutput();          // Added today.
    restartEngine();         // Added two weeks later.

It is so easy to make this mistake -- especially during merging -- that MISRA (Guidelines for the Use of the C++ Language in Critical Systems) made curly braces around if/else/do/while blocks a "required rule". Note that by following this rule, you are also protected from "dangling else" mistakes:

if (a < b)
    if (c < d)
        foo();
else    // This else actually belongs to the inner 'if'
    bar();

In general, I follow such "best practices" if they meet these criteria:

- They must solve a real problem
- They must not require any thinking

The "always-put-braces-around-blocks" rule does solve a real problem: I'm not aware of any compiler (not even Sun's Java compiler) that warns you about the mistake shown above. (Of course, static code analysis tools (like PC-Lint) do check for these errors, and many, many more, but unfortunately only few developers use them.)

Does it require any thinking? I don't think so. The only thing I have to remember is this: whenever I use 'if', 'else', 'for', 'while', 'do' I put curly braces around my code. That's all!

Maybe you have come across this convention:

if (MAX_RETRIES == retries) {  // Constant comes first.
    ...
}

Advocates of this style reason as follows: in C/C++ you can easily -- accidentally -- use an assignment operator when you actually wanted to use an equality operator. Since you cannot assign to a constant, the compiler will refuse to compile your buggy code:

if (MAX_RETRIES = retries) // Good: caught at compile-time.
if (retries = MAX_RETRIES) // Bad: not caught.

I must admit that I've never liked this convention. One reason is that it looks unnatural. Another is that it lacks symmetry; that is, it is not consistent with other comparison operators. What if you need to change the comparison some day to also include values greater than MAX_RETRIES?

if (MAX_RETRIES <= retries)     // Looks strange.
if (retries >= MAX_RETRIES)     // Looks natural, but requires an
                                // argument swap.

Or what if MAX_RETRIES is changed from a compile-time constant to a "normal" variable whose value is obtained from a config file? Do you swap then? In fact, this convention doesn't provide any help for mistakes in variable/variable comparisons, which occur also very frequently in day-to-day programming:

if (cfgMaxRetries = retries) // Bug!

For my taste, the utility of this convention is rather limited and it requires too much thinking: "If I compare a compile-time constant (or a literal) against a variable using the equality operator, I put the constant first". Isn't this a clear hindsight rule? It's a bit like attaching a tag saying "always lock the door" to your key.

Why not, instead, turn this into an habit: "If I do an equality comparison, I double check that there are two equal signs". That's not any more difficult to adopt, doesn't impair readability and has the additional benefit of working in all cases -- not just when comparing against constants.

I'm prepared to follow even conventions that look funny if they solve a real problem but I don't think that this convention does: today, most compilers produce warnings when you make this kind of mistake (for GCC it's -Wparenthesis, which is part of -Wall).

In C, there are numerous ways to make silly mistakes (in C++ there are many, many more). Instead of using questionable practices that address one tiny area, it is much better to enable all compiler warnings or -- even better -- use static code checkers like PC-Lint. In my opinion, that's the most worthwhile habit for safer coding of all.