Programming Practices – Is Short Circuit Evaluation Bad Practice?

code smelllanguage-agnosticoperatorsprogramming practices

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?

Best Answer

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.

`Maybe<T>`

public struct Maybe<T> where T : class
{
   public bool HasValue { get; private set; }

   private readonly T _value;
   public T Value
   {
       get
       {
           if(!HasValue){throw new InvalidOperationException();}
           return _value;
       }
   }

   public Maybe(T value) : this()
   {
       if(value==null) { throw new ArgumentNullException("value"); }
       HasValue = true;
       _value = value;
   }
}

(This could also be implemented other ways, such as an IEnumerable<T> with the constraint that it has exactly 0 or 1 items)

`NotNull<T>`

public class NotNull<T> where T : class
{
    public readonly T Value;

    public NotNull(T value)
    {
        if(item == null){ throw new ArgumentNullException("value"); }
        Value = value;
    }
}

Maybe<T> essentially just reimplements Nullable<T> for reference types. It has similar advantages and disadvantages to the ones described in your post. It's probably only valuable if you have a convention throughout your project to use it for any value where null is a valid value.

NotNull<T> is likely to be more directly what you want, as MainMa stated. By passing around values wrapped in this type you can remove the need to check whether a value is null in a guard clause. However, there's no way to completely prevent a NotNull<T> from itself being null. You can't make it a struct because of the requirement of a default constructor, which would leave Value null.

(EDIT: As others have pointed out, in C# 6 you will be able to have a parameterless constructor for a struct, but it will not be used when initializing arrays or getting the result of default(T), so you still can't really rely on the guarantee that Value won't be null)

So while both of these potentially add some value in signalling whether null should be considered valid or not, they're both limited in that they require constant adherence to a convention, in addition to the boilerplate and readability issues. Whether the trade-off is worth it would be a matter of judgement, but remember that code contracts or other static analysis tools also exist as a possible alternative way of addressing this issue.

Best Answer

Related Solutions

Is It Bad Practice to Check Class Instances by Interface in Java?

C# Programming Practices – Is Using Nullable for Non-Value Types a Bad Practice?

Maybe<T>

NotNull<T>

Related Topic

`Maybe<T>`

`NotNull<T>`