Wednesday, August 3, 2011

Object reference not set to an instance of an object

We all know, and love, this meaningful message from somewhere inside of a system.
Luckily I can say, in my last projects I became able to almost eliminate those failures - without a 100% unit test code coverage. (Those outside there who say, 100% code coverage by unit tests is a must must, please don't flame me. I'm a big fan of well tested software, but I guess I'm not yet ready for 100% coverage ;-).)

My solution for this old enemy was much easier than some might think. It was done by a simple coding convention. Don't get me wrong, I don't like hundreds of rules about how to write code. In my current project we've got less than 10 project internal coding conventions. Developing large software systems is some kind of a creative work and I think to many rules can eliminate the creativity of developers. I say every experienced developer should be able to write code that others can read and should be able to read code of others.

Our new rule to get rid of null reference exceptions was simply:

Get-methods never return null. They return a valid value or it throws an exception.

This does not mean that our system throws thousands of exceptions per minute. It also means that it is still possible to unsuccessfully try to get data from a method without an exception, we just do this different.

In old projects, code like this was a main part of daily business:
Customer customer = dataSource.GetCustomerById(1234);
if (customer != null) {
   customer.DoSomething();
}

This is nice code and works perfectly. Though, what if someone (me?) ever forgets to check if the method returned <null>? In this case it becomes only a question of time to see our old friend in a log file or - even worse - in a dialog shown to the user.
To avoid try-catch blocks over and over and avoid NullReferenceExceptions we simply established the TryGet pattern used by .NET classes like the Dictionary<T,V>. Instead of a possibly null returning Get-method we use a TryGet-method that returns a bool value indicating if our try to get something was successful or not.
public bool TryGetCustomerById(int id, out Customer customer) {
   using (var connection = CreateOpenConnection())
   using (var command = CreateProcCommand(connection, "GetCustomerById")) {
      command.Parameters.Add("@id", SqlDbType.Int).Value = id;

      using (var reader = command.ExecuteReader(CommandBehavior.SingleRow)) {
         if (reader.Read()) {
            customer = new Customer();
            customer.Id = id;
            customer.Name = (string)reader["Name"];
         }
         else {
            customer = null;
         }
      }
   }
   return customer != null;
}

Plus, if really needed, a Get-method that utilizes the TryGet-method and throws an exception if the search was unsuccessful.
public Customer GetCustomerById(int id) {
   Customer customer;
   if (!TryGetCustomerById(id, out customer))
      throw new EntityNotFoundException(
         string.Format("Cannot find customer with id {0}", id));
   return customer;
}

However, in many cases we don't need the Get-methods any more and in a few other cases we don't need the TryGet-methods since something must be wrong if the requested information is not available and a (meaningful) exception is a must.

This simple change to a TryGet-method changes our consuming library to something like this:
Customer customer;
if (dataSource.TryGetCustomerById(123, out customer)) {
   customer.DoSomething();
}
Some might argue "we still need the check for a result", and they are correct. But, this is a psychological thing. Calling a method that is called Try... clearly tells the consumer, that this call can return without a result. Even more. Sometimes, when it is okay if the method does not return a value, a developer might leave out the if-condition at the point where (s)he calls a TryGet-method. In those cases I saw other developers asking "But you call a Try-method and you don't check for the null value, are you sure??".

In cases where a method can return more than one item, like GetCustomersForCountry, we say it is valid to return an empty list. This is because of two reasons, an empty list is not null and most of the time those calls are followed by a loop that will not be entered for an empty list. However, if we call a method that returns a list and we only need the first item we should consider to add an additional method that returns only one item (like GetFirstCustomerForCountry). Another solution is an extension method "IEnumerable<T>.TryGetFirst(out T item)" and we are back in our style.

For sure, the samples I used here, to get data from some kind of data source are only one of many parts in a system where we find Get-methods. We use this pattern for all methods we write, independent if it works with a data source or any other objects. And we are doin' good with it.

As I said, every now and then we still see a NullReferenceExcption, but I guess there have been about 2 or 3 over the last 5 months of intensive development and testing. All in development or test environment, none in production. Plus, if this happens, we know there are only two options how to fix it. Either add the missing if-clause around the call of a TryGet-method (didn't happen over the last year) or change the null returning Get-method.

Probably I'm the only one who ever had problems with null reference exceptions but, for some reason, it feels like I'm not ;-). I can say we are truly on a point where we can call this old problem (almost) gone.

2 comments:

  1. I've been exploring different ways of getting around null references. Primarily, I've been focusing on creating class wrappers which require non-null references or throw on either initialization or retrieval of a null reference. Many of these can sometime end up awkward to use for whatever reason. For everything I've tried, the TryGet pattern you mention above seems to be the best approach. Along with throwing informative exceptions for null references for simple Get() accessors, this pattern really does seem to be the most natural so far and easy to work with.

    ReplyDelete
  2. Oh and one last thing - you are certainly not the only person who has had issues with NREs. They plague me on a day-to-day basis. This was a great article for me to get an idea of how to handle them. Thanks for the post!

    ReplyDelete