Should I null check in base class or derived class c#?

Question

I have a base class that has an abstract method;

public abstract void DoSomething(ClassA obj);

I have several derived classes each implementing this method and each checks obj for null (there is no case in any derived class where this should ever be null)e.g.

public override void DoSomething(ClassA obj)
{
    if(obj == null) throw new NullReferenceException();
    //Do other stuff...
}

It has occurred to me that I could refactor this;

public void DoSomething(ClassA obj)
{
    if(obj == null) throw new NullReferenceException();
    NextThing(obj)
}
protected abstract void NextThing(ClassA obj);

I now don't have to write a null check for each and every implementation because NextThing is "guaranteed" to have a none-null obj, but only if NextThing is only ever called by DoSomething. Is this a good idea or should I continue as I am?

Answer 1

It is a general rule that validity checks on arguments should be perfomed in any public method of any public class (visible to other assemblies). If you want to be even more defensive about arguments validity, check arguments of:

• protected methods of public classes
• public methods of internal classes
• protected methods of internal classes

The approach you suggested is a very good example and excellent use of Template Method Pattern. The main reason for me to say this is the sentence from your question:

I have several derived classes each implementing this method and each checks obj for null (there is no case in any derived class where this should ever be null)

So, you have the same pattern in all derived classes:

1. Perform a check
2. Do some action

You simply used the existing pattern and separated responsibilities of the public method, which is a good practice.

The only downside to this approach is if you happen to come across a situation where it is OK for a derived class to receive a null argument. Personally, the only situation where I see this as a valid scenario is if there is a valid default value for the argument to be provided by derived class if the argument is a null. In that case, you can flirt with breaking YAGNI and do something like this:

public abstract class BaseClass
{  
    public void DoSomething(ClassA obj)
    {
        ArgumentCheck(obj);
        NextThing(obj);
    }

    protected virtual void ArgumentCheck(ClassA obj)
    {
        if(obj == null) 
        {
            throw new ArgumentNullException();
        }
    }

    protected abstract void NextThing(ClassA obj);
}

Then, if such a need arises, you can always override the ArgumentCheck(ClassA obj) method to not throw exception in a particular class. The good thing here is that you can go with the original approach, and in case the need for this arises, you can perform this kind of refactoring without affecting the other classes.

Answer 2

A real answer to your question has to deal with other questions that really only you can answer:

• What is the intended usage of your base class?
• Is your base.DoSomething(someProp) supposed to be called first?
• How does the base class handle the null parameter?
• Is null a valid value? In some cases it is, so throwing exceptions would be wrong.

After that I will provide some guidelines that will help when debugging:

You don't always have to throw exceptions

Providing default values can be a good way to keep the system stable, particularly in cases where the parameter might intentionally be null. In those cases it might be better to simply return immediately. Exceptions are costly, and they are often overused.

Sometimes you can do just fine with Debug.Assert() to make it painfully obvious that something is wrong with the development build, which also gives the programmer the option to step in to the code with the debugger with the current stack. In the production build those checks go away, which can speed up the runtime--just be careful that the assertions don't have side effects you are depending on at run time.

Throw the Appropriate Exception

When you test a parameter and find it is null, you should throw new System.ArgumentNullException(nameof(parameter));. This helps weed out accidental NullReferenceExceptions from the things you checked for.

When parameters need more restrictions than simply existing (i.e. must be a positive number greater than 0, etc.) then throw the appropriate ArgumentException that helps the user understand what is wrong.

Providing the name of the parameter to the ArgumentNullException greatly expedites debugging--particularly when there are multiple parameters. The exception generates an easy to read message that includes the name of the parameter you provided, and a stack trace leading to the method that was called. Anything you can do to minimize time in a debugger will help you fix problems faster.

Make sure the parameter is not null before you access it

Every programmer is responsible to make sure that the parameter is valid before they do something with it. If you make it known that your base class is intended to be called first and it handles the null parameters, that still doesn't excuse the programmers who create subclasses from checking for null themselves if they violate that expectation.

Design for debugging

Do everything you can to ensure your code is as easy to debug and maintain as you can. The fact of the matter is that the hardest part of fixing bugs is finding where they happen in the first place. Often this is a trade-off between throwing exceptions and doing the best it can when the parameter provided is null.

I personally believe that subclassing is over-used, and can lead to unpredictable use cases because the clients of the code can have access to certain class state and do things that were never intended to begin with. If you use subclassing, then make the extension points abstract. If composition can get you what you want, it may be a better and more predictable pattern to extend the base functionalities.

Make extension points abstract instead of virtual

You see this pattern all over the place in the WinForms API. You have the method that promises to do something, like updating and redrawing. The base class has a pure abstract method with the prefix Do in front like this:

public abstract class MyBaseClass
{
    public void UpdateSomething(ClassA parameter)
    {
        if (parameter == null) throw new ArgumentNullException(nameof(parameter));

        DoUpdateSomething(parameter);
    }

    protected abstract void DoUpdateSomething(ClassA parameter);
}

This approach allows you to do all your sanity checks in the wrapper method, and only call the subclass implementation when everything is valid. This is going to be the only way you can ensure that the proper checks are made in the right order and give users of your base class the confidence that they are using it correctly.

Answer 3

If you use a null reference incorrectly you will get the exception anyway so why check? Concentrate on making sure that all functions handle exceptions of any kind and don't leave your system in an inconsistent or illegal state. That way you don't need to check nulls or any other things such as missing files etc. By all means throw exceptions when the domain state is in error but I'd not bother with these trivialities. It's the clean up and propagation that's important.