88

Something that I've known for a while but never considered is that in most languages it is possible to give priority to operators in an if statement based on their order. I often use this as a way to prevent null reference exceptions, e.g.:

if (smartphone != null && smartphone.GetSignal() > 50)
{
   // Do stuff
}

In this case the code will result in first checking that the object is not null and then using this object knowing that it exists. The language is clever because it knows that if the the first statement if false then there is no point even evaluating the second statement and so a null reference exception is never thrown. This works the same for and and or operators.

This can be useful in other situations too such as checking that an index is in the bounds of an array and such a technique can be performed in various languages, to my knowledge: Java, C#, C++, Python and Matlab.

My question is: Is this sort of code a representation of bad practice? Does this bad practice arise from some hidden technical issue (i.e. this could eventually result in an error) or does it lead to a readability issue for other programmers? Could it be confusing?

  • 69
    The technical term is short-circuit evaluation. This is a well-known implementation technique, and where the language standard guarantees it, relying on it is a good thing. (If it doesn't, it's not much of a language, IMO.) – Kilian Foth May 20 '15 at 9:05
  • 3
    Most languages let you chose what behavior you want. In C# the operator || short-circuits, the operator | (single pipe) does not (&& and & behave the same). As the short-circuit operators are used much more frequently, I recommend to mark usages of the non-short-circuit ones with a comment to explain why you need their behavior (mostly things like method invocations that all need to happen). – linac May 20 '15 at 12:58
  • 14
    @linac now putting something on the right-hand-side of & or | to make sure a side-effect happens is something I'd say certainly is bad practice: It won't cost you anything to put that method call on its own line. – Jon Hanna May 20 '15 at 14:01
  • 14
    @linac: In C and C++, && is short-circuit and & is not, but they're very different operators. && is a logical "and"; & is a bitwise "and". It's possible for x && y to be true while x & y is false. – Keith Thompson May 20 '15 at 19:40
  • 3
    Your specific example can be bad practice for another reason: what if it is null? Is it reasonable for it to be null here? Why is it null? Should the rest of the function outside this block execute if it's null, should the whole thing do nothing, or should your program stop? Papering an "if null" over every access (maybe due to a null reference exception you were in a hurry to get rid of) means that you can get rather far into the rest of the program, without ever noticing that the phone object initialization screwed up. And eventually, when you finally get to a bit of code that does not – Random832 May 21 '15 at 0:58
125

No, this is not bad practice. Relying on short-circuiting of conditionals is a widely accepted, useful technique--as long as you are using a language that guarantees this behavior (which includes the vast majority of modern languages).

Your code example is quite clear and, indeed, that is often the best way to write it. Alternatives (such as nested if statements) would be messier, more complicated, and therefore harder to understand.

A couple of caveats:

Use common sense regarding complexity and readability

The following is not such a good idea:

if ((text_string != null && replace(text_string,"$")) || (text_string = get_new_string()) != null)

Like many "clever" ways of reducing the lines in your code, over-reliance on this technique can result in hard-to-understand, error-prone code.

If you need to handle errors, there is often a better way

Your example is fine as long as it is a normal program state for smartphone to be null. If, however, this is an error, you should incorporate smarter error handling.

Avoid repeated checks of the same condition

As Neil points out, many repeated checks of the same variable in different conditionals are most likely evidence of a design flaw. If you have ten different statements sprinkled through your code that should not be run if smartphone is null, consider whether there is a better way to handle this than checking the variable each time.

This isn't really about short-circuiting specifically; it's a more general problem of repetitive code. However, it is worth mentioning, because it is quite common to see code that has many repeated statements like your example statement.

  • 12
    It should be noted that repeated if blocks checking if an instance is null in a short-circuit evaluation should probably be rewritten to perform that check only once. – Neil May 20 '15 at 9:20
  • 1
    @Neil, good point; I updated the answer. – user82096 May 20 '15 at 9:33
  • 2
    The only thing I can think to add is to note another optional way of handling it, which is really preferred if you are going to be checking for something in multiple different conditions: check to see if something is null in an if(), then return/throw - otherwise just keep going. This eliminates nesting and often can make the code more explicit and terse, as-in "this shouldn't be null and if it is then I'm outta here" - placed as early in the code as prudently possible. Great anytime you code something that checks a specific condition more than in one place in a method. – BrianH May 20 '15 at 20:41
  • 1
    @dan1111 I would generalize the example you gave as "A condition shouldn't contain a side effect(such as variable assignment above). Short-circuit evaluation doesn't change this and should not be used as short-hand for a side-effect that could have just as easily been placed in the body of the if to better represent the if/then relationship." – AaronLS May 20 '15 at 20:57
  • @AaronLS, I strongly disagree with the claim that "a condition shouldn't contain a side-effect". A condition shouldn't be unnecessarily confusing, but as long as it is clear code, I am fine with a side effect (or even more than one side effect) in a condition. – user82096 May 21 '15 at 13:30
26

Let's say you were using a C-style language with no && and needed to do the equivalent code as in your question.

Your code would be:

if(smartphone != null)
{
  if(smartphone.GetSignal() > 50)
  {
    // Do stuff
  }
}

This pattern would turn up a lot.

Now imagine version 2.0 of our hypothetical language introduces &&. Think how cool you'd think it was!

&& is the recognised, idiomatic means of doing what my example above does. It's not bad practice to use it, indeed it's bad practice not to use it in cases like the above: An experienced coder in such a language would be wondering where the else was or other reason for not doing things in the normal fashion.

The language is clever because it knows that if the the first statement if false then there is no point even evaluating the second statement and so a null reference exception is never thrown.

No, you are clever, because you knew that if the first statement was false then there is no point even evaluating the second statement. The language is dumb as a box of rocks and did what you told it to do. (John McCarthy was even more clever, and realised that short-circuit evaluation would be a useful thing for languages to have).

The difference between you being clever and the language being clever is important, because good and bad practice often come down to you being as clever as necessary, but no more clever.

Consider:

if(smartphone != null && smartphone.GetSignal() > ((range = (range != null ? range : GetRange()) != null && range.Valid ? range.MinSignal : 50)

This extends your code to check a range. If the range is null then it tries to set it by a call to GetRange() though that might fail so range might still be null. If range is not null after this, and it is Valid then its MinSignal property is used, otherwise a default of 50 is used.

This depends upon && too, but putting it in one line is probably too clever (I'm not 100% sure I have it right, and I'm not going to double-check because that fact demonstrates my point).

It's not && that is the problem here though, but the ability to use it to put a lot in one expression (a good thing) increases our ability to write hard-to-understand expressions unnecessarily (a bad thing).

Also:

if(smartphone != null && (smartphone.GetSignal() == 2 || smartphone.GetSignal() == 5 || smartphone.GetSignal() == 8 || smartPhone.GetSignal() == 34))
{
  // do something
}

Here I'm combining the use of && with a check for certain values. This isn't realistic in the case of phone signals, but does come up in other cases. Here, it's an example of my not being clever enough. If I'd done the following:

if(smartphone != null)
{
  switch (smartphone.GetSignal())
  {
    case 2: case 5: case 8: case 34:
      // Do something
      break;
  }
}

I'd have gained in both readability and also likely in performance (the multiple calls to GetSignal() would likely not have been optimised away).

Here again the problem isn't so much && as taking that particular hammer and seeing everything else as a nail; not using it let me do something better than using it does.

A final case that moves away from best practice is:

if(a && b)
{
  //do something
}

Compared to:

if(a & b)
{
  //do something
}

The classic argument as to why we might favour the latter is that there is some side-effect in assessing b that we want whether a is true or not. I disagree on that: If that side-effect is so important, then make it happen in a separate line of code.

However, in terms of efficiency either of the two are likely to be better. The first will obviously execute less code (not assessing b at all in one code path) which can save us whatever amount of time it takes to assess b.

The first though also has one more branch. Consider if we re-write it in our hypothetical C-style language with no &&:

if(a)
{
  if(b)
  {
    // Do something
  }
}

That extra if is hidden in our use of &&, but it is still there. And as such it's a case where there is branch prediction happening and potentially therefore branch mis-prediction.

For that reason the if(a & b) code can be more efficient in some cases.

Here I'd say that if(a && b) is still the best practice approach to start with: More common, the only one viable in some cases (where b will error if a is false) and faster more often than not. It's worth noting that if(a & b) is often a useful optimisation on it in certain cases though.

  • 1
    If one has try methods that return true in case of success, what do you think of if (TryThis || TryThat) { success } else { failure }? It's a less commmon idiom, but I don't know how to accomplish the same thing without code duplication or adding a flag. While the memory required for a flag would be a non-issue, from an analysis standpoint, adding a flag effectively splits code into two parallel universes--one containing statements that are reachable when the flag is true and the other one statements reachable when it's false. Sometimes good from a DRY perspective, but icky. – supercat May 20 '15 at 16:33
  • 5
    I see nothing wrong with if(TryThis || TryThat). – Jon Hanna May 20 '15 at 16:36
  • 3
    Damn fine answer, sir. – bishop May 21 '15 at 3:26
  • FWIW, excellent programmers including Kernighan, Plauger & Pike from Bell Labs recommend for the sake of clarity to use shallow control structures like if {} else if {} else if {} else {} rather than nested control structures like if { if {} else {} } else {}, even if it means evaluating the same expression more than once. Linus Torvalds said something similar: "if you need more than 3 levels of indentation, you're screwed anyway". Let's take that as a vote for short-circuit operators. – Sam Watkins May 21 '15 at 8:32
  • if (a && b) statement; is reasonably easy to replace. if (a && b && c && d) statement1; else statement2; is a real pain. – gnasher729 May 22 '15 at 19:32
10

You can take this further, and in some languages it's the idiomatic way to do things: you can use short circuit evualuation outside of conditional statements too, and they become a form of conditional statement themselves. E.g. in Perl, it is idiomatic for functions to return something falsy on failure (and something truthy on success), and something like

open($handle, "<", "filename.txt") or die "Couldn't open file!";

is idiomatic. open returns something nonzero on success (so that the die part after or never happens), but undefined on failure, and then the call to die does happen (that is Perl's way to raise an exception).

That is fine in languages where everybody does it.

  • 17
    The greatest contribution of Perl to language design is to provide multiple examples of how not to do it. – Jack Aidley May 20 '15 at 15:14
  • @JackAidley: I miss Perl's unless, and postfix conditionals (send_mail($address) if defined $address) though. – RemcoGerlich May 20 '15 at 17:57
  • 6
    @JackAidley could you provide a pointer to why 'or die' is bad? Is it just a naive way of handling exceptions? – bigstones May 21 '15 at 5:59
  • It is possible to do plenty of terrible things in Perl, but I fail to see the problem with this example. It is simple, short, and clear--exactly what you should want from error handling in a scripting language. – user82096 May 21 '15 at 13:26
  • 1
    @RemcoGerlich Ruby inherited some of that style: send_mail(address) if address – bonh May 21 '15 at 15:03
6

While I generally agree with dan1111's answer, there is one particularly important case it does not cover: the case where smartphone is used concurrently. In that scenario, this sort of pattern is a well known source of hard to find bugs.

The problem is that short circuit evaluation is not atomic. Your thread can check that smartphone is null, another thread can come in and nullify it, and then your thread promptly tries to GetSignal() and blows up.

But of course, it only does that when the stars align and your threads' timing is just right.

So while this sort of code is very common and useful, it is important to know about this caveat so that you can accurately prevent this nasty bug.

3

There are plenty of situations where I want to check one condition first, and only want to check a second condition if the first condition succeeded. Sometimes purely for efficiency (because there is no point checking the second condition if the first already failed), sometimes because otherwise my program would crash (your check for NULL first), sometimes because otherwise the results would be awful. (IF (I want to print a document) AND (printing a document failed)) THEN display an error message - you don't want to print the document and check whether printing failed if you didn't want to print a document in the first place!

So there was a HUGE need for guaranteed short circuit evaluation of logical expressions. It wasn't present in Pascal (which had non-guaranteed short circuit evaluation) which was a major pain; I have first seen it in Ada and C.

If you really think this is "bad practice", then please take any moderately complex bit of code, rewrite it so that it will work fine without short circuit evaluation, and come back and tell us how you liked it. This is like being told that breathing produces carbon dioxide and asking if breathing is a bad practice. Try without it for a few minutes.

  • "Rewrite it so that it will work fine without short circuit evaluation, and come back and tell us how you liked it." -- Yes, this brings back memories of Pascal. And no, I didn't like it. – Thomas Padron-McCarthy May 21 '15 at 12:21
2

One consideration, not mentioned in other answers: sometimes having these checks can hint at a possible refactoring to the Null Object design pattern. For example:

if (currentUser && currentUser.isAdministrator()) 
  doSomething();

Could be simplified to just be:

if (currentUser.isAdministrator())
  doSomething ();

If currentUser is defaulted to some 'anonymous user' or 'null user' with a fallback implementation if the user isn't logged in.

Not always a code smell, but something to consider.

-4

I'm going to be unpopular and say Yes, it is bad practice. IF the functionality of your code is relying on it to implement conditional logic.

ie

 if(quickLogicA && slowLogicB) {doSomething();}

is good, but

if(logicA && doSomething()) {andAlsoDoSomethingElse();}

is bad, and surely no-one would agree with?!

if(doSomething() || somethingElseIfItFailed() && anotherThingIfEitherWorked() & andAlwaysThisThing())
{/* do nothing */}

Reasoning :

Your code errors if both sides are evaluated rather than simply not performing the logic. It has been argued that this is not a problem, the 'and' operator is defined in c# so the meaning is clear. However, I contend that this is not true.

Reason 1. Historical

Essentially you are relying on (what was originally intended as) a compiler optimization to provide functionality in your code.

Although in c# the language specification guarantees the boolean 'and' operator does short circuiting. This is not true of all languages and compilers.

wikipeda lists various languages and their take on short circuiting http://en.wikipedia.org/wiki/Short-circuit_evaluation as you can there are many approaches. And a couple of extra problems I don't go into here.

In particular, FORTRAN, Pascal and Delphi have compiler flags which toggle this behavior.

Therefore : You may find you code works when compiled for release, but not for debug or with an alternate compiler etc.

Reason 2. Language differences

As you can see from wikipedia, not all languages take the same approach to short circuiting, even when they specify how the boolean operators should handle it.

In particular VB.Net's 'and' operator does not short circuit and TSQL has some peculiarities.

Given that a modern 'solution' is likely to include a number of languages, such as javascript, regex, xpath etc you should take this into account when making your code functionality clear.

Reason 3. Differences within a language

For example VB's inline if behaves differently from its if. Again in modern applications your code may move between, for example code behind and an inline webform, you have to be aware of the differences in meaning.

Reason 4. Your specific example of null checking the parameter of a function

This is a very good example. In that it is something everyone does, but is actually bad practice when you think about it.

Its a very common to see this kind of check as it reduces your lines of code considerably. I doubt anyone would bring it up in a code review. (and obviously from the comments and rating you can see it's popular!)

However, it fails best practice for more than one reason!

  1. Its very clear from numerous sources, that best practice is to check your parameters for validity before you use them and to throw an exception if they are invalid. Your code snippet dosn't show the surrounding code, but you should probably be doing something like:

    if (smartphone == null)
    {
        //throw an exception
    }
    // perform functionality
    
  2. You are calling a function from within the conditional statement. Although your function's name implies that it simply calculates a value, it can hide side effects. eg

    Smartphone.GetSignal() { this.signal++; return this.signal; }
    
    else if (smartphone.GetSignal() < 1)
    {
        // Do stuff 
    }
    else if (smartphone.GetSignal() < 2)
    {
        // Do stuff 
    }
    

    best practice would suggest:

    var s = smartphone.GetSignal();
    if(s < 50)
    {
        //do something
    }
    

If you apply these best practices to your code you can see that your opportunity to achieve shorter code through the use of the hidden conditional disappears. Best practice is to do something, to handle the case where smartphone is null.

I think you will find that this is also the case for other common usages. You have to remember we also have :

inline conditionals

var s = smartphone!=null ? smartphone.GetSignal() : 0;

and null coalescence

var s = smartphoneConnectionStatus ?? "No Connection";

which provide similar short code length functionality with more clarity

numerous links to support all my points:

https://stackoverflow.com/questions/1158614/what-is-the-best-practice-in-case-one-argument-is-null

Constructor parameter validation in C# - Best practices

Should one check for null if he does not expect null?

https://msdn.microsoft.com/en-us/library/seyhszts%28v=vs.110%29.aspx

https://stackoverflow.com/questions/1445867/why-would-a-language-not-use-short-circuit-evaluation

http://orcharddojo.net/orchard-resources/Library/DevelopmentGuidelines/BestPractices/CSharp

examples of compiler flags that affect short circuiting:

Fortran : "sce | nosce By default, the compiler performs short circuit evaluation in selected logical expressions using XL Fortran rules. Specifying sce allows the compiler to use non-XL Fortran rules. The compiler will perform short circuit evaluation if the current rules allow it. The default is nosce."

Pascal : "B - A flag directive that controls short-circuit evaluation. See Order of evaluation of operands of dyadic operators for more information about short-circuit evaluation. See also the -sc compiler option for the command-line compiler for more information (documented in the User's Manual)."

Delphi : "Delphi supports short circuit evaluation by default. It can be turned off using the {$BOOLEVAL OFF} compiler directive."

protected by gnat May 21 '15 at 13:25

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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