Developer insists if statements shouldn't have negated conditions, and should always have an else block

Question

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?

Answer 1

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))

Answer 2

Regarding the first rule, this is an example of useless typing. Not only does it take longer to type, it will cause huge amounts of confusion to anyone reading the code. If the code isn't needed, don't write it. This would even extend to not having a populated else in your case as the code returns from the if block:

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

doSomethingElse(); // no else needed
return somethingelse;

Regarding the second point, it's good to avoid boolean names that contain a negative:

bool doorNotOpen = false; // a double negative is hard to reason
bool doorClosed = false; // this is much clearer

However, extending this to not testing for a negative again, as you point out, leads to more useless typing. The following is far clearer than having an empty if block:

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

Answer 3

1. An argument in favor of empty else statements.

I oftentimes use (and argue for) something akin to that first construct, an empty else. It signals to readers of the code (both human and automated analysis tools) that the programmer has put some thought into the situation. Missing else statements that should have been present have killed people, crashed vehicles, and cost millions of dollars. MISRA-C, for example, mandates at least a comment saying that the missing final else is intentional in an if (condition_1) {do_this;} else if (condition_2) {do_that;} ... else if (condition_n) {do_something_else;} sequence. Other high reliability standards go even further: with a few exceptions, missing else statements are forbidden.

One exception is a simple comment, something along the lines of /* Else not required */. This signals the same intent as does the three line empty else. Another exception where that empty else is not needed is where it's blatantly obvious to both readers of the code and to automated analysis tools that that empty else superfluous. For example, if (condition) { do_stuff; return; } Similarly, an empty else is not needed in the case of throw something or goto some_label¹ in lieu of the return.

2. An argument for preferring if (condition) over if (!condition).

This is a human factors item. Complex boolean logic trips many people up. Even a seasoned programmer will have to think about if (!(complex || (boolean && condition))) do_that; else do_this;. At a minimum, rewrite this as if (complex || (boolean && condition)) do_this; else do_that;.

3. This does not mean one should prefer empty then statements.

The second section says "prefer" rather than "thou shalt". It is a guideline rather than a rule. The reason for that guideline to prefer positive if conditions is that code should be clear and obvious. An empty then clause (e.g., if (condition) ; else do_something;) violates this. It is obfuscated programming, causing even the most seasoned of programmers to back up and re-read the if condition in its negated form. So write it in the negated form in the first place and omit the else statement (or have an empty else or comment to that effect if mandated to do so).

---

¹I wrote that then clauses that end with return, throw or goto do not require an empty else. It's obvious that the else clause is not needed. But what about goto? As an aside, safety-critical programming rules sometimes disallow early return, and almost always disallows throwing exceptions. They do however allow goto in a restricted form (e.g., goto cleanup1;). This restricted use of goto is the preferred practice in some places. The Linux kernel, for example, is chockfull of such goto statements.

Answer 4

I use an empty else-branch (and sometimes an empty if-branch) in very rare cases: When it is obvious that both the if and else part should be handled somehow, but for some non-trivial reason the case can be handled by doing nothing. And therefore anyone reading the code with the else action needed would immediately suspect that something is missing and waste their time.

if (condition) {
    // No action needed because ...
} else {
    do_else_action()
}

if (condition) {
    do_if_action()
} else {
    // No action needed because ...
}

But not:

if (condition) {
    do_if_action()
} else {
    // I was told that an if always should have an else ...
}

Answer 5

All else being equal, prefer brevity.

What you don't write, nobody has to read and understand.

While being explicit can be useful, that's only the case if it makes obvious without undue verbosity that what you wrote is really what you wanted to write.

---

So, avoid empty branches, they are not only useless but also uncommon and thus lead to confusion.

Also, avoid writing an else-branch if you exit directly out of the if-branch.

A useful application of explicitness would be putting a comment whenever you fall through switch-cases // FALLTHRU, and using a comment or an empty block where you need an empty statement for(a;b;c) /**/;.

Answer 6

There is no hard and fast rule about positive or negative conditions for an IF statement, not to my knowledge. I personally prefer coding for a positive case rather than a negative, where applicable. I most certainly will not do this though, if it leads me to make an empty IF block, followed by an ELSE full of logic. If such a situation arose, it would take like 3 seconds to refactor it back to testing for a positive case anyway.

What I really don't like about your examples though, is the completely unnecessary vertical space taken up by the blank ELSE. There is quite simply no reason whatsoever to do this. It doesn't add anything to the logic, it doesn't help document what the code is doing, and it doesn't increase readability at all. In fact, I would argue that the added vertical space could possibly decrease readability.

Answer 7

Explicit else block

I disagree with this as a blanket statement covering all if statements but there are times when adding an else block out of habit is a good thing.

An if statement, to my mind, actually covers two distinct functions.

If we are supposed to do something, do it here.

Stuff like this obviously does not need an else part.

    if (customer.hasCataracts()) {
        appointmentSuggestions.add(new CataractAppointment(customer));
    }
    if (customer.isDiabetic()) {
        customer.assignNurse(DiabeticNurses.pickBestFor(customer));
    }

and in some cases insisting on adding an else might mislead.

    if (k > n) {
        return BigInteger.ZERO;
    }
    if (k <= 0 || k == n) {
        return BigInteger.ONE;
    }

is not the same as

    if (k > n) {
        return BigInteger.ZERO;
    } else {
        if (k <= 0 || k == n) {
            return BigInteger.ONE;
        }
    }

even though it is functionally the same. Writing the first if with an empty else may lead you to the second result which is unnecessarily ugly.

If we are checking for a specific state, it is often a good idea to add an empty else just to remind you to cover that eventuality

        // Count wins/losses.
        if (doors[firstChoice] == Prize.Car) {
            // We would have won without switching!
            winWhenNotSwitched += 1;
        } else {
            // We win if we switched to the car!
            if (doors[secondChoice] == Prize.Car) {
                // We picked right!
                winWhenSwitched += 1;
            } else {
                // Bad choice.
                lost += 1;
            }
        }

Remember that these rules apply only when you are writing new code. IMHO The empty else clauses should be removed before checkin.

---

Test for true, not for false

Again this is good advice at a general level but in many cases this makes code unnecessarily complex and less readable.

Even though code like

    if(!customer.canBuyAlcohol()) {
        // ...
    }

is jarring to the reader, but making it

    if(customer.canBuyAlcohol()) {
        // Do nothing.
    } else {
        // ...
    }

is at least as bad, if not worse.

I coded in BCPL many years ago and in that language there is an IF clause and an UNLESS clause so you could code much more readably as:

    unless(customer.canBuyAlcohol()) {
        // ...
    }

which is significantly better, but still not perfect.

---

My personal process

Generally, when I am writing new code I will often add an empty else block to an if statement just to remind me that I have not yet covered that eventuality. This helps me avoid the DFS trap and ensures that when I review the code I notice that there is more to do. However, I usually add a TODO comment to keep track.

  if (returnVal == JFileChooser.APPROVE_OPTION) {
    handleFileChosen();
  } else {
    // TODO: Handle case where they pressed Cancel.
  }

I do find that generally I use else rarely in my code as it can often indicate a code smell.

Answer 8

For the first point, I have used a language that forced IF statements to be used this way (in Opal, the language behind a mainframe screen scraper for putting a GUI front end on to mainframe systems), and with only one line for the IF and the ELSE. It wasn't a pleasant experience!

I would expect any compiler to optimise out such additional ELSE clauses. But for live code it is not adding anything (in development it can be a useful marker for further code).

A time I do use something like these extra clauses is when using CASE / WHEN type processing. I always add a default clause even if it is empty. This is long term habit from languages that will error if such a clause is not used, and forces a thought as to whether things really should just drop through.

Long ago mainframe (eg, PL/1 and COBOL) practice it was accepted that negative checks were less efficient. This could explain the 2nd point, although these days there are massively more important efficiency savings which are ignored as micro optimisations.

Negative logic does tend to be less readable, although not so much on such a simple IF statement.

Answer 9

I would second the stand of most answers that empty else blocks are virtually always a harmful waste of electronic ink. Don't add these unless you have a very good reason to do so, in which case the empty block should not be empty at all, it should contain a comment explaining why it's there.

The issue about avoiding negatives deserves some more attention though: Typically, whenever you need to use a boolean value, you need some blocks of code to operate when it's set, and some other blocks to operate when it's not set. As such, if you enforce a no-negatives rule, you enforce either having if() {} else {...} statements (with an empty if block!), or you create a second boolean for each boolean that contains its negated value. Both options are bad, as they confuse your readers.

A helpful policy is this: Never use a negated form within a boolean's name, and express negation as a single !. A statement like if(!doorLocked) is perfectly clear, a statement like if(!doorUnlocked) knots brains. The later type of expression is the thing you need to avoid at all cost, not the presence of a single ! in an if() condition.

Answer 10

I would say it's definitely bad practice. Adding else statements is going to add a whole bunch of pointless lines to your code that do nothing and make it even more difficult to read.

Answer 11

There's another pattern that hasn't been mentioned yet about handling the second case that has an empty if-block. One way to deal with it would be to return in the if block. Like this:

void foo()
{
    if (condition) return;

    doStuff();
    ...
}

This is a common pattern for checking error conditions. The affirmative case is still used, and the inside of it is no longer empty leaving future programmers to wonder if a no-op was intentional or not. It can also improve readability since you might be required to make a new function (thus breaking large functions into smaller individual functions).

Answer 12

There is one point when considering the "always have an else clause" argument that I haven't seen in any other answer: it can make sense in a functional programming style. Sort of.

In a functional programming style, you deal in expressions rather than statements. So every code block has a return value - including an if-then-else expression. That would, however, preclude an empty else block. Let me give your an example:

var even = if (n % 2 == 0) {
  return "even";
} else {
  return "odd";
}

Now, in languages with C style or C style inspired syntax (such as Java, C# and JavaScript, just to name a few) this looks weird. However it looks much more familiar when written as such:

var even = (n % 2 == 0) ? "even" : "odd";

Leaving the else branch empty here would cause a value to be undefined - in most cases, not what we want to be a valid case when programming functionality. Same with leaving it out completely. However, when you're programming iteratively I set very little reason to always have an else block.

Answer 13

...an if statement should be followed by an else statement, whether there is something to put into it or not.

I disagree if (a) { } else { b(); } should be re-written as if (!a) { b(); } and if (a) { b(); } else { } should be re-written as if (a) { b(); }.

However, it's worth pointing out that I rarely have an empty branch. This is because normally I log that I went into the otherwise empty branch. This way I can develop purely off of log messages. I rarely use a debugger. In production environments you don't get a debugger; it's nice to be able to troubleshoot production issues with the same tools that you use to develop.

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

I have mixed feelings about this. A disadvantage to having doorClosed and doorOpened is it potentially doubles the number of words/terms you need to be aware of. Another disadvantage is over time the meaning of doorClosed and doorOpened may change (other developers coming in after you) and you might end up with two properties that are no longer precise negations of each other. Instead of avoiding negations, I value adapting the language of the code (class names, variable names, etc.) to the business users' vocabulary and to the requirements I'm given. I wouldn't want to make up a whole new term to avoid a ! if that term only has meaning to a developer that business users won't understand. I want developers to speak the same language as the users and the people writing requirements. Now if new terms simplify the model, then that's important, but that should be handled before the requirements are finalized, not after. Development should handle this problem before they start coding.

I am prone to doubt my instinct.

It's good to continue to question yourself.

Is it a good/bad/"doesn't matter" practice?

To some degree a lot of this doesn't matter. Get your code reviewed and adapt to your team. That's usually right thing to do. If everyone on your team wants to code a certain way, it's probably best to do it that way (changing 1 person - yourself - takes less story points than changing a group of people).

Is it common practice?

I can't remember ever seeing an empty branch. I do see people adding properties to avoid negations, though.

Answer 14

I'm only going to address the second part of the question and this comes from my point of view of working in industrial systems and manipulating devices in the real world.

However this is in someways an extended comment rather than an answer.

When it is stated

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.

From my point of view the assumption is being made here that the door state is a binary state when in fact it is at least a ternary state:

1. The door is open.
2. The door is neither fully open nor fully closed.
3. The door is closed.

Thus depending on where a single, binary digital sensor is located you can only surmise one of two concepts:

1. If the sensor is on the open side of the door: The door is either open or not open
2. If the sensor is on the closed side of the door: The door is either closed or not closed.

And you can not interchange these concepts through the use of a binary negation. Thus doorClosed and !doorOpened are likely not synonymous and any attempt to pretend that they are synonymous is erroneous thinking that assumes a greater knowledge of system state than actually exists.

In coming back to your question I would support verbiage that matches the origin of the information that the variable represents. Thus if the information is derived from the closed side of the door then go with doorClosed etc. This may imply using either doorClosed or !doorClosed as needed in various statements as required, resulting in potential confusion with the use of the negation. But what it does not do is implicitly propagate assumptions on the state of the system.

---

For discussion purposes for the work I do, the amount of information I have available about the state of the system depends on the functionality required by the overall system itself.

Sometimes I only need to know if the door is closed or not closed. In which case only a single binary sensor will suffice. But at other times I do need to know that the door is open, closed or in transition. In such cases there would either be two binary sensors (at each extreme of door motion - with error checking against the door being simultaneously open and closed) or there would be an analogue sensor measuring how 'open' the door was.

An example of the first would be the door on a microwave oven. You enable the operation of the oven based on the door being closed or not closed. You do not care how open the door is.

An example of the second would be a simple motor driven actuator. You inhibit driving the motor forward when the actuator is fully out. And you inhibit driving the motor in reverse when the actuator is fully in.

Fundamentally the number and type of sensors comes down to running a cost analysis of the sensors against a requirements analysis of what is needed to achieve the required functionality.

Answer 15

Concrete style rules are always bad. Guidelines are good, but in the end there's often cases where you can make stuff clearer by doing something that doesn't quite follow the guidelines.

That said, there's a lot of hate for the empty else "it wastes lines" "extra typing", that are just bad. You could make the argument for moving the brackets onto the same line if you really need the vertical space, but if that's an issue you shouldn't be putting your { on a separate line anyway.

As mentioned elsewhere, having the else block is very useful to show that you explicitly want nothing to happen in the other case. After doing a lot of functional programming (where else is required), I've learned that you should always be considering the else when writing the if, although as @OldCurmudgeon mentions there are really two different use cases. One should have an else, one shouldn't. Sadly it's not something you can always tell at a glance, let alone with a linter, hence the dogmatic 'always put an else block'.

As for the 'no negatives', again, absolute rules are bad. Having an empty if can be weird, especially if it's the type of if that doesn't need an else, so writing all that rather than a ! or an ==false is bad. That said, there are a lot of cases where the negative makes sense. A common example would be caching a value:

static var cached = null

func getCached() {
    if !cached {
        cached = (some calculation, etc)
    }

    return cached
}

if the actual (mental/english) logic involves a negative, so should the if statement.