December 4, 2012

Null + Type Checking: The Bad, The Ugly, and The Good

It is often the case that one must check for nulls and types at the same time. Over the last couple of weeks I've been both reading and writing code that checks for nulls and types for various subclasses throughout a codebase. Let's review some of the ways that one might perform such checks in order to execute conditional code branches:

The above code is just bad. Really bad. OMGWTF?!? bad. Yet, it's probably what I come across most often in code reviews. The reason it's bad is because the cast will throw an unhandled InvalidCastException if BaseClassInstance is not, in fact, an instance of SubclassType. Okay, so we know we need to handle that exception now, right? Let's try it again.

Alright, so technically the above code works and meets our requirements, but it's inelegant. We don't really want to be catching an InvalidCastException as a means of determining whether the types match. We can do better by removing the explicit cast.

Whoa... that's a lot of code to just check for a type and value. I mean, yes, it's technically correct and meets requirements, but could you scan that really quickly in a review and know what the hell was going on?

The above code is semantically the same as the previous example, but is clearly much more concise. The is keyword checks that an instance of a reference type is not null, and that it is of the specified type. So from now on, use the is keyword whenever you want to check for nulls and types in the same conditional expression.


Ilia Jerebtsov said...

The first snippet will work if you change it to:

if ((BaseClassInstance as SubclassType) != null) {

Brian Driscoll said...

Yes, but it's still 2 steps (an implicit cast followed by a null check). IMO using the is operator is both more concise and more elegant than any multi-step solution.

Anonymous said...

I think the real problem is that you're even in the situation of having to do something based on the type of a variable in the first place. Code that needs to do this could be at best described as a necessary evil - but one would never call it good. It's fundamentally unmaintainable and should be replaced with something polymorphic.

If you absolutely must downcast, then odds are that you need to use something on the interface of the subclass - in which case using 'as' can do you much better:
SubclassType sub;
if((sub = baseClassInstance as SubclassType) != null)

Brandon Furtwangler said...

There is a good chance you will need to use the instance of SubclassType inside the body. If that's the case it's better to do:

var subclassInstance = BaseClassInstance as SubclassType;
if (subclassInstance != null) {
// use subclassInstance

That way you avoid doing the cast twice (first 'is' and later a cast/'as'), which is relatively expensive at runtime (not O(1)).