186

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?

21
  • 65
    Is it possible your coworker comes from a background where life and limb might be at stake? I believe I've seen a few mentions of this sort of thing in systems where a crap ton of testing and automatic analysis is done on code and where getting it wrong could literally cost someone their life.
    – jpmc26
    Jun 9, 2017 at 3:47
  • 12
    What does your company code style guide say? Jun 10, 2017 at 11:12
  • 6
    Partial response to your "Secondly" question. If one of the action blocks is long and the other short, test a condition that puts the short block first, so that it's less likely to be missed when reading the code. That condition could be a negation, or a renamed to make it a test for true. Jun 10, 2017 at 13:39
  • 9
    Resharper would have something to tell your friend, along the line of "Your code is redundant and useless, do you want me to delete/refactor it?"
    – BgrWorker
    Jun 12, 2017 at 7:15
  • 4
    to back up the comment by @jpmc26, it is worth noting that this is a MISRA rule.
    – Baldrickk
    Jun 12, 2017 at 14:06

15 Answers 15

205

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))
25
  • 183
    Empty blocks always look like mistakes to me. It'll immediately draw my attention and I'll ask 'Why is this here?'
    – Nelson
    Jun 8, 2017 at 14:24
  • 57
    @Neil Negating a condition does not necessarily require any additional CPU time. C compiled to x86 could just swap the jump instruction from JZ (Jump if Zero) for if (foo) to JNZ (Jump if Not Zero) for if (!foo).
    – 8bittree
    Jun 8, 2017 at 16:32
  • 78
    "create additional properties with clear names": Unless you have deeper analysis of the domain, this may be changing the global scope (the original class) to solve a local problem (the application of a specific business rule). One thing that can be done here is to create local booleans with the desired state, such as "var isSlave = !this.IsMaster" and apply your if with the local booleans instead of creating several other properties. So, take the advice with a grain of salt and take some time to analyse the domain before creating new properties.
    – Machado
    Jun 8, 2017 at 17:09
  • 87
    It's not about "three lines of code", it's about writing for the appropriate audience (someone with at least a basic grasp of programming). An if is already explicit about the fact that the condition could be false, or you wouldn't be testing for it. An empty block makes things less clear, not more, because no reader is primed to expect it, and now they have to stop and think about how it could have gotten there.
    – hobbs
    Jun 8, 2017 at 19:29
  • 15
    @Mark that's less clear, even with the comment, than saying if (!commonCase) { handle the uncommon case }, though.
    – Paul
    Jun 8, 2017 at 23:02
65

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;
}
8
  • 131
    So... you could say that double negative is a no-no? ;)
    – Patsuan
    Jun 8, 2017 at 15:20
  • 8
    @Patsuan, very clever. I like that. :)
    – David Arno
    Jun 8, 2017 at 15:24
  • 6
    I feel like not many people would agree about how if-else with returns should be handled, and some might even say it shouldn't be handled the same in every scenario. I don't have a strong preference myself, but I can definitely see the argument behind many other ways of doing it. Jun 8, 2017 at 19:02
  • 7
    Clearly, if (!doorNotOpen) is awful. So names should be as positive as possible. if (! doorClosed) is perfectly fine. Jun 9, 2017 at 21:16
  • 3
    With regards to your door example, the two example names are not necessarily equivalent. In the physical world doorNotOpen is not the same as doorClosed as there is a transitional state between being open and closed. I see this all the time in my industrial applications were boolean states are determined by physical sensors. If you have a single sensor on open side of the door then you can only say the door is open or not open. Likewise if the sensor is on the closed side, you can only say closed or not closed. Also the sensor itself determines how the logic state is affirmed.
    – Peter M
    Jun 13, 2017 at 14:48
51

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



1I 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.

18
  • 6
    ! also is easily overlooked. It is too terse. Jun 10, 2017 at 11:13
  • 4
    No, an empty else doesn't make it clearer that the programmer has put some thought in it. It leads to more confusion "Were there some statements planned and they forgot or why is there an empty clause?" A comment is much clearer.
    – Simon
    Jun 10, 2017 at 20:26
  • 11
    @Simon: A typical form is else { /* Intentionally empty */ }, or something along those lines. The empty else satisfies the static analyzer that mindlessly looks for rules violations. The comment informs readers that the empty else is intentional. Then again, seasoned programmers typically omit the comment as unnecessary -- but not the empty else. High reliability programming is a domain of its own. The constructs look rather strange to outsiders. These constructs are used because of lessons from the school of hard knocks, knocks that are very hard when life & death or $$$$$$$$$ are at hand. Jun 11, 2017 at 2:28
  • 2
    I don't understand how empty then clauses are a bad thing when empty else clauses are not (assuming we choose that style). I can see if (target == source) then /* we need to do nothing */ else updateTarget(source)
    – Bergi
    Jun 11, 2017 at 17:05
  • 2
    @ThorbjørnRavnAndersen nope, not is a keyword in C++, as are other bitwise and logical operators. See alternative operator representations.
    – Ruslan
    Jun 12, 2017 at 14:40
36

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 ...
}
8
  • 5
    This. I find the statement if test succeeds -> no action needed -> else -> action needed to be semantically very different to the statement if it applies that the opposite of a test outcome is true then an action is needed. I'm reminded of Martin Fowler's quote: "anyone can write code computers can understand; good programmers write code humans can understand". Jun 10, 2017 at 15:17
  • 2
    @TasosPapastylianou That's cheating. The opposite of 'if test succeeds -> no action needed' is 'if test does not succeed -> action needed'. You padded it with about 10 words.
    – Jerry B
    Jun 11, 2017 at 17:55
  • @JerryB it depends on the "test". Sometimes negation is (linguistically) straightforward, but sometimes it isn't, and would lead to confusion. In those cases, it's clearer to talk about "negation / opposite of the outcome being true" vs "outcome not being true"; however the point is that it would be even clearer to introduce an 'empty' step, or invert the meaning of the variable names. This is typical in "de morgan" situations: e.g. if ((missing || corrupt) && !backup) -> skip -> else do stuff is far clearer (+different implied intent) than if ((!missing && !corrupt) || backup) -> do stuff Jun 12, 2017 at 0:58
  • 1
    @TasosPapastylianou You could write if(!((missing || corrupt) && !backup)) -> do stuff or better if((aviable && valid) || backup) -> do stuff. (But in a real example you must check if the backup is valid before use it) Jun 12, 2017 at 12:34
  • 2
    @TasosPapastylianou where is there double negatives in what I said? Sure, if you were using uncorrupted as a variable name then you would have a clear point. Though when you come to mixed boolean operators like this, it may be better to have temporary variables to use in the if: file_ok = !missing && !corrupt; if(file_ok || backup) do_stuff(); which I personally feel makes it even clearer.
    – Baldrickk
    Jun 12, 2017 at 16:18
23

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) /**/;.

12
  • 7
    // FALLTHRU Ugh, not in SCREAMING CAPS or with txtspk abbrevs. Otherwise, I agree and follow this practice myself. Jun 9, 2017 at 7:11
  • 7
    @CodyGray -- This is one place where SHOUTING is a good idea. Most switch statements end each case with break. Failure to have that break has led to some spectacular software failures. A comment that shouts to readers of the code that the fall-through is intentional is a good thing. That static analysis tools can look for this specific pattern and not complain about a fall through situation makes it an even better thing. Jun 9, 2017 at 9:03
  • 1
    @Wossname: I just checked my vim, and it doesn't recognize any variation of FALLTHRU. And I think, for good reason: TODO and FIXME comments are comments that need to be grepable because you want to be able to ask your git grep which known defects you have in your code, and where they are located. A fallthrough is not a defect, and there is no reason to grep for it, what so ever. Jun 11, 2017 at 1:02
  • 4
    @DavidHammen No, shouting is not a good idea: If you have a //fallthrough in place of an expected break;, that should be enough for any developer to understand immediately. Also, the //fallthrough does not talk about a defect, it simply signals that the circumstances have been considered, and that the break; which appears to be missing, is indeed intended to not be there. This is a purely informational purpose, and nothing to shout about. Jun 11, 2017 at 15:17
  • 1
    @cmaster -- That shouting is not a good idea is your opinion. Other people's opinions differ. And just because your configuration of vim doesn't recognize FALLTHRU doesn't mean that other people haven't configured vim to do so. Jun 11, 2017 at 16:50
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.

5
  • 2
    That unnecessary vertical space is a consequence of mandates for always using braces, for disallowing missing else, and for using Allman style for indentation. Jun 9, 2017 at 11:03
  • Well, I would think that always using braces is a good thing, because it makes a developers intentions explicit - no room for guesswork when reading over their code. That might sound silly, but trust me, especially when mentoring junior developers, mistakes related to missing braces do happen. I've personally never been a fan of Allman style indentation, for exactly the reason you mention: unnecessary vertical space. But that's just my opinion and personal style. It's just what I learned initially, so it has always felt more comfortable to read.
    – Langecrew
    Jun 9, 2017 at 13:44
  • Personal style: regardless of which brace style I use (Allman, 1TB, ...), I always use braces. Jun 9, 2017 at 14:31
  • Complex conditionals or ones you think might be difficult to understand can almost always be solved by being re-factored into their own methods/functions. Something like this if(hasWriteAccess(user,file)) could be complex underneath, but at a glance you know exactly what the result should be. Even if it's just a couple conditions, e.g. if(user.hasPermission() && !file.isLocked()) a method call with an appropriate name gets the point across, so negatives become less of an issue. Jun 9, 2017 at 19:47
  • Agreed. Then again, I'm a big fan of refactoring whenever possible :)
    – Langecrew
    Jun 9, 2017 at 19:54
5

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.

4
  • Your handling of "empty" blocks reads like standard stubbing out code when implementing thngs... Jun 9, 2017 at 10:05
  • The unless block is a good suggestion, and hardly confined to BCPL.
    – tchrist
    Jun 11, 2017 at 2:29
  • @TobySpeight Oops. It somehow escaped my attention that what I've written as ... was actually return. (In fact, with k=-1, n=-2, the first return is taken but this is largely moot.) Jun 12, 2017 at 9:12
  • Modern Perl has if and unless. What's more, it also allows these after a statement, so you can say print "Hello world!" if $born; or print "Hello world!" unless $dead; and both will do exactly what you think they do.
    – user
    Jun 12, 2017 at 12:49
1

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.

4
  • 2
    I see, so it was common practice at some point.
    – Patsuan
    Jun 9, 2017 at 12:06
  • @Patsuan - yes, to an extent. Although that was when mainframe assembler was a serious alternative for performance critical code.
    – Kickstart
    Jun 9, 2017 at 14:30
  • 1
    "Long ago... it was accepted that negative checks were less efficient." Well, they are unless the compiler optimizes if !A then B; else C to if A then C; else B. Computing the negation of the condition is an extra CPU instruction. (Though as you say, this is unlikely to be significantly less efficient.) Jun 12, 2017 at 10:03
  • @DavidRicherby And if we are talking in general, some languages can make such optimizations really hard. Take C++ with overloaded ! and == operators, for example. Not that anyone should actually do that, mind you...
    – user
    Jun 12, 2017 at 12:52
1

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.

0
1

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.

0

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.

1
  • 1
    I hear you have never worked for an employer who grades your performance based on SLOCs produced per period...
    – user
    Jun 12, 2017 at 12:53
0

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

0

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

0

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.

2
  • "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:" and "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." contradict each other. Also, I don't think the question is about doors and sensors at all.
    – Sanchises
    Jun 14, 2017 at 8:41
  • @Sanchises The statements are not contradictory as you have taken them out of context. The first statements is in the context that two opposing binary measurements of a ternary state can not be interchanged via binary negation. The second statement is in the context that only partial knowledge of a ternary state may be sufficient for an application to correctly work. As for the doors, the OPs question referenced doors being open and closed. I am just pointing out an obvious assumption that can influence the how data is treated.
    – Peter M
    Jun 14, 2017 at 11:21
-1

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.

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