Should I check if argument is null if I'm going to use it immediately?

Question

I'm so used to check if a method's argument is null (then proceed to throw an exception) that I almost don't even think about it anymore. If the argument is a reference type, it's there:

if(arg == null)
    throw new ArgumentNullException(nameof(arg));

But what if I'm going to use arg immediately? Should I check anyway? I mean, if I don't, the envinroment will throw for me (a NullReferenceException) anyway.

For instance:

public int DoStuff(object o)
{
    return o.GetHashCode();
}

I could write add the null check easily:

public int DoStuff(object o)
{
    if(o == null)
        throw new ArgumentNullException(nameof(o));
    return o.GetHashCode();
}

But in both cases a exception will be thrown (in almost the exact same line, for debugging purpose). The only difference is the type.

The question: On public methods with a single reference type argument, if I'm going to use the argument immediately, should I still check it if it's null?

Answer 1

I suggest checking, because you have two Exception types:

1. Unexpected NullReferenceException - something went wrong, and you have to debug your own routine
2. ArgumentNullException - the argument is null, and it's caller that's wrong (and not your code which reacts right)

throwing ArgumentNullException is kind of contract: I'll do the thing in case argument is correct:

  // An exaggerated example 
  public int DoStuff(SomeObject o) {
    if (o == null)
      throw new ArgumentNullException(nameof(o));
    else if (o.Disposed)
      throw new ArgumentException(nameof(o), "o must not be disposed")  
    else if (o.Id > 100)
      throw new ArgumentOutOfRangeException("Id ...", nameof(o));  

    // o meets the contract, we accept it and thus we guarantee the output

    return o.SomeMethod();
  }

This kind of validation is typical for public (protected) methods since they're exposed to outer world and can face any argument; however, in case of private method you can omit the contract: any caller is within the class that implements the method.

Answer 2

Well, it depends ;) This is my take on it, and the question is (kinda) subjective.

IMO, if the code throws a NullReferenceException, it means that the code itself has no internal consistency. It is a sign of lazyness and should be avoided at all costs. If a method throws a NullReferenceException, it is method's fault.

An ArgumentNullException, on the other hand, means that the method cannot "do it's stuff" if the argument is null. It becomes responsibility of the invoker to guarantee that the parameter is not null, or handle graciously the exception.

Bottom line: an ArgumentNullException is a contract, a NullReferenceException is a bug.

Answer 3

I suppose if you are immediately dereferencing the variable, you could debate either way, but I would still prefer the ArgumentNullException. It is much more explicit about what is going on. The exception contains the name of the variable that was null, whereas a NullReferenceException does not.

Answer 4

I strongly suggest you check for null at the top of the method.

Think of that as a "contract" that specifies the following:

In order for this method to execute, this argument must be non-null.

Doing the null check is great documentation. You could go one step further an use XML comments e.g.

/// <summary>
/// 
/// </summary>
/// <param name="arg1"></param>
/// <exception cref="ArgumentNullException">If <paramref name="arg1"/> is NULL.</exception>
private static void Foo(object arg1)
{
}

Answer 5

I'd say it depends more on how you handle the errors, not how they're thrown. An example will help. Imagine this is the method instead of what you have:

public static int DoStuff(string o) {
    return Int.Parse(o);
}

If you have this, then it really doesn't matter.

public static void main(string[] args)
{
    try {
        Console.Write("Enter a number: ");
        value = DoStuff(Console.ReadLine());
    }
    catch(Exception ex) {
        Console.WriteLine("An exception was thrown.  Contents: " + ex.ToString());
    }
}

Either way your program flow is the same. But if you have below:

public static void main(string[] args)
{
    try {
        int value = 0;
        do {
            try {
                Console.Write("Enter a non-zero number to stop: ");
                value = DoStuff(Console.ReadLine());
            }
            catch(ArgumentException ex) {
                Console.WriteLine("Problem with input, try again.  Exception message was: " + ex.Message);
                continue; // Or just nothing, will still loop
            }
        } while(value == 0);
    }
    catch(Exception ex) {
        Console.WriteLine("An exception was thrown.  Contents: " + ex.ToString());
    }
}

Basically, if your error handling doesn't care which exception type it is, then it's just extra code that probably doesn't help you. But if you handle specific things differently, then it may be worth it to test and throw the more specific type of exception.

But I do agree with Dmitry's post above that a public method matters a lot more than a private one. Your signature for the method is somewhat like a contract. It's best to enforce it.

Answer 6

There are a few different scenario's I think you should consider:

1. The method is called from your own program. You don't expect a null value. You use the object somewhere down the line and if it's null, it will still change the state.
2. The method is called from your own program. You don't expect a null value. If it's a null, it won't change the state and everything will continue to be consistent.
3. The method is exposed through a library or API. An unknown developer X calls the method with whatever you want.
4. You just want to know that an argument is not null while implementing stuff. To protect yourself and your team members.

Points (1) and (2) are about exception safety. Basically this states that regardless of your input, the state of your data structures should be consistent. In most cases this is resolved through a simple check; in more complex scenario's it might involve a rollback or a more complex piece of machinery. So:

1. If you add a null value to a database, you don't want it to mess up your database. Now, you can of course solve null values by simply checking your assumption. If the argument is null, throw an ArgumentNullException because that best describes the scenario.
2. If you call a method that doesn't expect a null value and it won't influence the rest of your system, I usually put a comment somewhere and let nature run its course. A NullReferenceException tells me enough about the nature of the bug.
3. If it's an API, people expect certain behavior (e.g. a ArgumentNullException). Moreover, as a developer you shouldn't trust the big bad outside world so you should check anyways.
4. If you just want to protect yourself and your team members, use an Assertion. Assertions are usually implemented in such a way, that they don't end up in release code (or at least have no impact on your speed), will automatically trigger a breakpoint and will fail your unit test. They're also easy to recognize so they won't add too much bloat to your code.

To summarize, I wouldn't just put checks everywhere; they tend to make your code unreadable and bloaty. That said, if I would put a check to ensure exception safety, it's best to use the exception that best describes the error.

Answer 7

Follow Einstein - "make things as simple as possible, but no simpler". Does that code do the same thing without a line of code? If so, it is probably best to remove it.