Monday, 13 January 2014

Guideline #3. The condition in an "if" statement should be a simple boolean expression with no side effects

Guideline

  • The expression inside an if statement should be boolean.
  • It should be simple and readable.
  • It should have no side effects.
  • It should not include assignments or variable declarations.

    Discussion

    1. A boolean is a boolean

    In C++, every statement is an expression of a certain type. It may be an ordinary type, such as boolint or a user-defined type such as Customer), or a special type called void which basically means "no type".

    The language rules establish which types are fundamental in the language. You don't need to invent your own bool type, for example, because the language has one. So you won't use invented types such as BOOL or Bool, will you?

    The language rules also determine its syntax, of course. They say what types are expected at each part of an instruction. Thework of the people who make compilers is to follow these rules so that they create C++ programs which execute the code you write in the right way. Your part of the work is to know these rules so that after being compiled, your program does not only what you wrote (it will, if the compiler is correct), but also what you wanted to do when you were writing it.

    Different languages have different rules. The C language, for example, does not have a native boolean type. When that language was created, if was defined as taking an int as an argument, so that you would write code such as if (count). This worked, but was not natural.

    The natural argument of a conditional expression is a boolean expression. If the boolean expression is true, the code block whichs follow the if will be executed. Otherwise, it won't.

    In C++ the expression inside the pair of brackets will be understood as a boolean expression. Some people carry the habit from their C days and pass an as int an argument to the if. Other people even use a pointer, so that the if will check whether it is NULL and will evaluate to true in that case. Don't do it - it makes the code less readable and a step closer to being error prone.

    Guideline #2 advised you to avoid implicit type conversions. This is a very good place to start applying it. The if statement requires a bool, so why pass it anything else?

    2. And nothing else than a boolean

    A boolean expression, that's all. Don't overcomplicate things by putting several actions together. Don't use complex commands which may succeed or not, returning a boolean which indicates success, as conditions inside if statements. Don't write assignments or, worse, variable declarations in the condition, either. In short, keep it simple. Don't do anything in your code that you wouldn't do when writing an instructions list for someone to follow. If you do, you'll (or someone else will) soon be asking yourself (or themselves) where is the software failing, only to discover that you need to debug into each and every complex step which you one day decided to write together inside of an if condition.

    While we're at it, everything which has been said here also applies to the conditions inside loop instructions such as for, while and do ... while .

    This second part (keep it simple, no side effects) also applies to the expression which is evaluated in a switch instruction, altough in that case of course you're not using a boolean expression, but an integer or an enum.

    Example

    Don't

    if (!isConnected() || !saveToFile())
    showMessage("Error saving to file.");

    What have you done? You have omitted the square brackets in the code block which is executed if the condition is true! Please don't do that. I didn't mention that in the guideline, but I took it for granted. Put the square brackets in their place and then we'll continue the discussion.

    if (!isConnected() || !saveToFile())
    {
    showMessage("Error saving to file.");
    }

    Thank you.

    In this example you mixed two possible complex operations (one may be a query, isConnected; the other is a command) in the condition, so you won't easily know which one failed when you see the message. At least you used a boolean expression, which is good; but it is not a simple one, and it has side effects. Let's see how we can improve it.

    Do

    if (!isConnected())
    {
    showMessage("Cannot not save to file: Not connected.");
    }
    else
    {
    bool saveSucceeded = saveToFile();
    if (!saveSucceeded)
    {
    showMessage("Error saving to file.");
    }
    }

    There are a few alternatives, most of them better than the original code. In this case at least I separated the two possible causes of error, giving them different error messages (the error in saveToFile probably needs more detailed information, but that is beyond the scope of this example). It's easy to debug the code step by step (or imagine it being debugged) and figure out what's going on.

    I assumed that isConnected is simple query and left it inside the condition; had it been a complex operation, I'd isolate it in its own line just as I did with saveToFile.

    Which leads us to the most important point: a command such as saveToFile should be called in its own line, not buried inside the conditional expression of an if statement. It's better to separate the list of things that you need to do (check whether the object, whatever it is is connected, and then save to file) from a conditional action which you will perform if the result of one of these actions is not satisfactory.

    Bibliography

    This materials referenced in this section discuss the issue described above in more detail.

    • Steve McConnell: Code Complete, 2nd Edition, Microsoft Press, Redmond (Washington, USA), 2004.
    This book discusses conditionals in Chapter 15. In particular, see Section 15.1 "if statements" (pages 355-360) and the checklist at the end of the chapter (page 365).

    No comments:

    Post a Comment