Whose fault is a NullReferenceException?

Question

I'm currently working on a class which exposes an internal List through a property. The List shall and can be modified. The problem is, entries in the internal list could be set to null from outside the class.

My code actually looks like this:

class ClassWithList
{
    List<object> _list = new List<object>();

    // get accessor, which however returns the reference to the list,
    // therefore the list can be modified (this is intended)
    public List<object> Data
    {
        get
        {
            return _list;
        }
    }

    private void doSomeWorkWithTheList()
    {
        foreach(object obj in _list)
            // do some work with the objects in the list without checking for null.
    }
}

So now in the doSomeWorkWithTheList() I could always check whether the current list entry is null or I could just asume that the person using this class doesn't have the great idea to set entries to null.

So finally the questions end up in: Whose fault is a NullReferenceException in this case? Is it the fault of the class developer not checking everything for null (which would make code generally - not only in this class - more complex) or is it the fault of the user of this class, as setting a List entry to null doesn't really make sense? I'd tend to generally not check values for null except in some really special cases. Is this a bad style or de facto standard / standard in praxis?

I know there's probably no ultimate answer for this, I'm just missing enough experience for such thing and therefore wondering what other developers think about such cases and want to hear what's done in reality about checking null (or not).

Answer 1

If you expose the list, you allow the user of your class to add null references at will.
If you allow users to add null references at will, your class needs to be prepared for null references in the list.

private void doSomeWorkWithTheList()
{
    foreach(object obj in _list)
        if (obj != null)
           // do some work with the object
}

If you don't like that, don't expose the list (which should be exposed as IList<T> btw) but return a list-like collection that doesn't allow null references to be added.

Answer 2

Sorry to be blunt, but if your code falls over inelegantly, it's your fault. How about you encapsulate the list, and expose a null-checking add method if the rest of your code has this requirement? Maybe your class could implement IList, and internally store the list you are going to process - then in the Add method, you just either throw a NullArgumentException or ignore the null and continue...

Answer 3

You shouldn't trust a user not to do something like this. Sometimes a list entry will get set to null. These things happen. I would try and catch it whenever possible. If you expose the list there is always a posibility that a null reference will occur, so you should account for this.

Its not your fault as such, but it doesn't matter whether its actually the user's fault - you will be blamed.

Answer 4

If you allow nulls inside the list, then you need to check for them. IF you want to assign "blame" here, then it's this class, not the users of the class. It would be simple via some getters and setters to prevent null from every making it into the list if they didn't belong in it.

Answer 5

If setting the List to NULL does not make sense for the use of the class, then the fault lies with the class designer, who is responsible for preventing that operation on the class.

Answer 6

It's the fault of the person who's trying to use the List. You always have to assume that it could be null and so you should check for it.

This goes into Defensive Programming and how you have to protect your code from what other people do.

Code Complete in Chapter 8 goes into this concept.

Answer 7

This depends on what you want to do with the list in the doSomeWorkWithTheList() and what you would do if a value is null.

I would either check for null and throw an Exception myself (with a message like "Invalid List Value, null is not allowed") or not allow null values by creating a subclass of Collection<> which throws if someone uses AddItem with a null value.

By providing the API you also provide a contract. If the contract says "null values are not allowed" then it's the responsibility of the user of your API not to set anything to null and your responsibility to make sure the contract is followed.

Answer 8

If you don't want nulls in the list, you're going to throw an exception somewhere. The question is: where? When the user is trying to add to the list or when you're trying to process it?

I would throw a ArgumentException at the point the consumer of the class was trying to add to the list, as what is the point of allowing them to add the item if you can't work with it anyway. But you will want to throw an exception. Don't let the consumer assume everything worked merely because they weren't informed otherwise.

Answer 9

Just filtering the list to avoid NREs is bad practice, it hides bugs in the client code. The code is bound to malfunction without any good way to diagnose why. Let it bomb.

Exposing a collection object for the client code to party on isn't a very good practice either. Derive your class from IList<object> (does it have to be object?). You'll delegate the IList's methods to a private List<object> field in your class. But you can override the indexer and implement the Add method to verify that the client code isn't putting nulls in the collection. If that's really necessary.

Answer 10

Use Collection<T>, it is built-in customisable wrapper for List<T> you can then override InsertItem and SetItem and throw an exception if the item is null.

Answer 11

Add code contracts. Then you can check for nulls at method start, return value and through out using invariant checking. Then depending on what version of visual studio you have, you can check at compile time and run time.

Answer 12

I really do not like inserting nulls into collections of any kind other than arrays.

It seems I differ with a lot of posters here. To me, anybody who sticks a null into a collection that is a property of some other object deserves what he gets later on.

When I coded my custom hashtable (System.Generic.Dictionary has some problems I won't go into here) I decided to make lookup return null on not found. Since I don't insert nulls this made my life easier.