Coding Style – Should If Statements Avoid Negated Conditions?

coding-styleconditions

I have an acquaintance, a more seasoned developer than me.
We were talking about programming practices and I was taken aback by his approach on 'if' statements.
He insists on some practices regarding if statements that I find rather strange.

Firstly, an if statement should be followed by an else statement, whether there is something to put into it or not. Which leads to code looking like this:

if(condition) 
{
    doStuff();
    return whatever;
}
else
{
}

Secondly, it's better to test for true values rather than false. That means that it's better to test a 'doorClosed' variable instead of a '!doorOpened' variable

His argument is that it makes clearer what the code is doing.

Which confuses me quite a bit, as a combination of those two rules can lead him to write this kind of code if he wants to do something when the condition isn't met.

if(condition)
{
}
else
{
    doStuff();
    return whatever;
}

My feeling about this is that it's indeed very ugly and/or that the quality improvement, if there is any, is negligible. But as a junior, I am prone to doubt my instinct.

So my questions are: Is it a good/bad/"doesn't matter" practice? Is it common practice?

Best Answer

Explicit else block

The first rule just pollutes the code and makes it neither more readable, nor less error-prone. The goal of your colleague — I would suppose — is to be explicit, by showing that the developer was fully aware that the condition may evaluate to false. While it is a good thing to be explicit, such explicitness shouldn't come at a cost of three extra lines of code.

I don't even mention the fact that an if statement isn't necessarily followed by either an else or nothing: It could be followed by one or more elifs too.

The presence of the return statement makes things worse. Even if you actually had code to execute within the else block, it would be more readable to do it like this:

if (something)
{
    doStuff();
    return whatever;
}

doOtherThings();
return somethingElse;

This makes the code take two lines less, and unindents the else block. Guard clauses are all about that.

Notice, however, that your colleague's technique could partially solve a very nasty pattern of stacked conditional blocks with no spaces:

if (something)
{
}
if (other)
{
}
else
{
}

In the previous code, the lack of a sane line break after the first if block makes it very easy to misinterpret the code. However, while your colleague's rule would make it more difficult to misread the code, an easier solution would be to simply add a newline.

Test for true, not for false

The second rule might make some sense, but not in its current form.

It is not false that testing for a closed door is more intuitive than testing for a non-opened door. Negations, and especially nested negations, are usually difficult to understand:

if (!this.IsMaster || (!this.Ready && !this.CanWrite))

To solve that, instead of adding empty blocks, create additional properties, when relevant, or local variables.

The condition above could be made readable rather easily:

if (this.IsSlave || (this.Synchronizing && this.ReadOnly))
Related Topic