Handling null-references in C# logic

Question

Let's say I have an API method with can be used to calculate the sum of all orders made by a specific customer:

Amount CalculateOrderSum(int customerId)
{
    // Perform authentication to make sure caller has access to customerId

    // Retrieve customer with id customerId

    // Retrieve all orders related to the customer

    // Retrieve details for different orders (not always, depending on state)
}

At any point in this function, some system administrator may purge old items from the system. This means that one of the below can happen while the method above is running:

• The authentication will fail, because the customer no longer exists
• The customer can't be loaded, since it was deleted
• The customer can be loaded, but a millisecond later the orders are deleted and cannot be retrieved.
• Retrieving order details works for some orders, but fails for others since they have been deleted mid-processing.

I want the application to return a friendly error message when any of this happens, rather than returning a NullReferenceException or similar.

As I see it, there's some different approaches to add error-handling for this logic:

1. I could introduce a lot of null-checks throughout the code for example: if (customer != null) throw OrderRetrievalFailedException("customer is a goner."). Since all the data in the database can be purged at any time, this would lead to quite a lot of if's spread throughout the code (which seems to get messy)
2. I could change the purging functionality to mark customers or orders as deleted (rather than actually removing the database rows). This way the function could still do its work because the data will still be there. The issue here is that we actually want to purge old data for different reasons (less attack surface and performance considerations for example).
3. I could change all methods to throw if an object can't be loaded (so GetOrders(customerId) could throw CustomerNotFoundException if the customer cannot be loaded) which would be catched in the CalculateOrderSum function and an error given to the user. So basically the code would have to be littered with if (something == null) throw new SomeException.
4. I could introduce some global locking mechanism, so that a customer can't be deleted while any one is reading any of its data. The issue here is that our system is distributed so we would need to implement a central locking mechanism. Also, I have a bad experience with locking of database rows in use-cases like this in high-traffic database.

All of these approaches feels quite convoluted and tricky to get right to me, and it means that the "main success flow" of the code will be littered with handling of exception scenarios. I'm leaning towards alternative 3, but I would to hear if there's some other "standard and robust" way of handling this.

(I'm using C#, but I assume that the same issue would apply to users of for example Java or C++)

Answer 1

Rather than have your C# code perform all this computation and thus have to handle all the error conditions you describe, why not move it all to a stored procedure that is transactional? That way, you can lock the DB for example, to ensure it doesn't change whilst you perform your multiple steps.

Answer 2

Honestly, I would just set up your project so it handles errors gracefully, probably by taking the user to a custom error page (via an error handler) that says something like "Whoops, somthing went wrong. Try again later."

I admire the idea of trying to prevent the error in the first place (and @DavidArno's idea with transactions is spot on). But just know that at some point there will just be some errors you can't possibly code around, like, say, having your database server just die mid-request. Sure, you could put in thousands of checks all over the place for every conceivable situation, but then your code becomes an unmaintainable nightmare. Would you want to have a code base like this (psuedocode, also exaggerated to make a point):

function GetUserName(userId) {
    get connection
    check to make sure connection failure one didn't happen
    check to make sure connection failure two didn't happen
    ....
    check to make sure connection failure nine didn't happen
    check to make sure connection failure ten didn't happen
    try grabbing the user
    check that no errors were thrown because the db may have died
    repeat previous checks again
    check that user is not null
    make sure ....
    return user.Name;
}

What should be a simple, couple line function has now ballooned into a huge monster. I don't want to imagine something more complicated. (This isn't to say that some error checking isn't appropriate. At some point, however, you do need to assume that stuff will work.)

Sometimes you just have to be tolerant of life happening. A generic oops message suffices when there isn't anything more specific to say that is worth having more complicated code.

Answer 3

They're called "Guard Clauses." It would look something like this:

if (!userHasAccess) throw new UnauthorizedException();
if (customer == null) throw new NotFoundException("customer");
if (orders.Count == 0) return 0;
if (anOrderHasFailed) throw new NotFoundException("Order " + orderNumber.ToString());

Not that difficult, really, and your code will be more robust for the effort.

For those who are concerned about race conditions, you're going to get a NullReferenceException when you try to dereference a property on the null object anyway, so if you want to, you can just skip the null checks.

In practice, such race conditions are quite rare; if your customer suddenly disappears, you have larger problems.

Answer 4

Welcome to the world of coding in an object-oriented language that isn't using a framework to guard against any null-reference pathways. This pain is one of the primary reasons people look into paradigms from other languages to deal with this uncertainty. For example, immutable objects, once instantiated, can safely be treated as never-null if your instantiation code guarantees a proper non-null return, and proper non-null hydration of all that objects properties.

Here's a few of my personal thoughts about your code sample above:

If a given method can take any customer ID as a param, then it can't always return the same kind of object (an "Amount" object) for a case where the ID is valid versus invalid. And throwing an exception for every possible invalid value in every method is ugly Exception-driven-programming, which I personally dislike, because it breaks normal function flow and is sorta like GOTO in that regard. I'd suggest returning something like a Message<T> object, where the Amount class is the T for the call about, and the Message class has fields for things like bool Success and string[] Errors. That way, your method can just detect or try/catch on a failed Customer load, and return that message object without even trying to process a valid Amount return.

However, you are still left with a Message<Amount> object that might have a null Amount object packed inside. To deal with this, you have a few options. Firstly, just leave it null and set up your code to always check for the Success field before operating on the <T> inside. Or, you could create your return objects as struct's, which, being value types, are never null. HOWEVER, that comes with a world of consequences, so make sure you're OK with value types being passed around if so. Thirdly, you could make use of the NullObject pattern, wherein you define a way to construct an Amount object, but of a sub-type called something like NullAmount, and your consuming code will have to know how to look out for these NullAmount objects and react accordingly.

If you are dead-set against doing any kind of null or conditional checking when you consume the results of that method call, then probably throwing exceptions is your best bet, but I would strongly encourage you to looking to the Message pattern, or the NullObject pattern above as better solutions. Regardless, C#/Java allow object references to be null by design, and so some level of legwork is involved in checking for and dealing with, this most famous of Billion Dollar Mistakes.

Answer 5

The described scenario is actually a really good fit for doing exception handling correctly -- to stop a process and recover from exceptional errors.

Exceptions should bubble up, to the first place where you can continue intelligently, taking a new direction in light of the error. Don't be afraid to let that be all the way out of the application. You can only recover to where you can -- trying to do it too early just leads to another failure.

Spending all of your time doing error checks, that you really can't do anything with anyway, is counter productive. Embrace the exceptions, don't try to fight them.