4

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?

  • 1
    Possible duplicate of Is Template Pattern a good way to implement DRY? – gnat Aug 2 '17 at 11:36
  • 2
    Is there any reason to do an explicit check? – Deduplicator Aug 2 '17 at 11:39
  • 2
    In general, I'd avoid public virtual methods. – CodesInChaos Aug 2 '17 at 12:01
  • 10
    Sidenote: You're throwing the wrong exception. It should be ArgumentNullException. – CodesInChaos Aug 2 '17 at 12:02
  • Voting to close this as the answers so far show it's very much an opinion-based question. – David Arno Aug 2 '17 at 14:33
4

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.

  • 2
    Shotgunning parameter checking code everywhere is solving the wrong problem in the wrong place. Limit the error checking code to where it is absolutely required otherwise you're just creating noise and degrading the code base. If you can provide a reasonable default, then do so... – Robbie Dee Aug 2 '17 at 14:29
  • I did not say "everywhere". I specifically listed where it should be done. If this is a class that is published to external clients, then there is no way to ensure that they perform this check in their code. On the other hand, whether the check should throw exception or provide a meaningful default value depends on particular case. I edited my reply to address this. – Vladimir Stokic Aug 2 '17 at 14:38
  • 3
    NOTE: the CLR throws NullReferenceException(), programmers should use ArgumentNullException instead. See my points about designing for debugging and why throwing ArgumentNullException helps. – Berin Loritsch Aug 2 '17 at 14:58
  • 1
    @BerinLoritsch Thank you. You are completely right. Copy/Paste error. I corrected it in my answer. – Vladimir Stokic Aug 2 '17 at 15:05
  • @VladimirStokic I am aware that you've shortlisted where to check but blindly adding defensive code without being 100% sure it adds value is cargo cult programming at worst, and cruft at best. – Robbie Dee Aug 2 '17 at 17:17
6

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.

  • +1 Some good points here. DoUpdateSomething is missing its return type BTW... – Robbie Dee Aug 2 '17 at 14:16
  • @BerinLoritsch many useful points, the final point you make describes the approach I was planning on taking. – mark_h Aug 2 '17 at 15:39
  • There's several correct ways of handling it, hopefully I've given you some ammunition for other approaches in your code as needed. Having written parsers, background processors, UI code, etc. the appropriateness of each approach depends on what you are doing. – Berin Loritsch Aug 2 '17 at 15:43
  • The last point is actually the correct answer for the specific case the OP posited. – Robbie Dee Aug 2 '17 at 17:19
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.

  • This is the only real answer to the question. Defensive programming is painting lipstick on the null-pig. Just stop doing it already. Write decent tests for your code to ensure you don't pass nulls around, rather than waste time protecting parts of your code from poorly tested other parts. – David Arno Aug 2 '17 at 20:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.