Short circuit evaluation, is it bad practice?

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

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.

Answer 6

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.

Answer 7

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