77

Suppose I have a method that takes an object of some kind as an argument. Now say that if this method is passed a null argument, it's a fatal error and an exception should be thrown. Is it worth it for me to code something like this (keeping in mind this is a trivial example):

void someMethod(SomeClass x)
{
    if (x == null){
        throw new ArgumentNullException("someMethod received a null argument!");
    }

    x.doSomething();
}

Or is it safe for me to just rely on it throwing NullException when it calls x.doSomething()?

Secondly, suppose that someMethod is a constructor and x won't be used until another method is called. Should I throw the exception immediately or wait until x is needed and throw the exception then?

Vivek Nuna
  • 25,472
  • 25
  • 109
  • 197
Jason Baker
  • 192,085
  • 135
  • 376
  • 510
  • 6
    It's also worth noting that the constructor of `ArgumentNullException` that you're using here doesn't take a full message as a parameter. Instead, it takes in `paramName` which is the name of the argument that was null. See [MSDN for the full definition](http://msdn.microsoft.com/en-us/library/wssa019h(v=vs.110).aspx). – Drew Gaynor Oct 08 '14 at 16:06
  • 1
    http://programmers.stackexchange.com/questions/121121/how-does-throwing-an-argumentnullexception-help – Gopi Apr 13 '15 at 05:27
  • 2
    See Code Analysis rule [CA1062: Validate arguments of public methods](https://msdn.microsoft.com/en-us/library/ms182182.aspx). – DavidRR Apr 05 '16 at 15:24
  • 1
    My only issue with that code would be the error message in the ArgumentNullException not actually specifying which argument was null. – Søren Ullidtz Nov 07 '16 at 04:19

15 Answers15

71

I prefer the ArgumentNullException over the NullReferenceException that not checking the argument would provide. In general, my preference is to always check for nullity before trying to invoke a method on a potentially null object.

If the method is a constructor, then it would depend on a couple of different factors: is there also a public setter for the property and how likely is it that the object will actually be used. If there is a public setter, then not providing a valid instance via the constructor would be reasonable and should not result in an exception.

If there is no public setter and it is possible to use the containing object without referencing the injected object, you may want to defer the checking/exception until its use is attempted. I would think that the general case, though, would be that injected object is essential to the functioning of the instance and thus an ArgumentNull exception is perfectly reasonable since the instance can't function without it.

brainoverflow98
  • 1,138
  • 11
  • 28
tvanfosson
  • 524,688
  • 99
  • 697
  • 795
  • 10
    Yeah, it's not a matter of preference. ArgumentNullException is correct and NullReferenceException isn't. NullReferenceException is for when a null object is accessed, such as a field or property of it. – JamesBrownIsDead Nov 10 '09 at 22:22
  • +1. You might use [code contracts][1] to minimize the code overhead of `ArgumentNullException`: `Contract.Requires(x != null, nameof(x));` I think this is the shortest way without significant runtime overhead. It still has the duplicate reference to x but that's the only drawback. [1]: https://learn.microsoft.com/en-us/dotnet/framework/debug-trace-profile/code-contracts – Marcel Dec 02 '17 at 15:00
  • If you are thinking "buy why is it correct?", Martin Fowler said something that resonated with me. "Bugs are easier to find and fix, so fewer go into production." https://www.martinfowler.com/ieeeSoftware/failFast.pdf – Gabe Nov 14 '19 at 19:08
41

I always follow the practice of fail fast. If your method is dependent on X and you understand X might be passed in null, null check it and raise the exception immediately instead of prolonging the point of failure.

2016 update:

Real world example. I strongly recommend the usage of JetBrains Annotations.

[Pure]
public static object Call([NotNull] Type declaringType, 
                          [NotNull] string methodName, 
                          [CanBeNull] object instance)
{
    if (declaringType == null) throw new ArgumentNullException(nameof(declaringType));
    if (methodName == null) throw new ArgumentNullException(nameof(methodName));

Guard statements have been vastly improved with C# 6 providing the nameof operator.

Pang
  • 9,564
  • 146
  • 81
  • 122
Chris Marisic
  • 32,487
  • 24
  • 164
  • 258
16

No excuse not to make the check these days. C# has moved on and you can do this very neatly using a discard and a null coalescing operator:

_ = declaringType ?? throw new ArgumentNullException(nameof(declaringType));
_ = methodname ?? throw new ArgumentNullException(nameof(methodName));
AndyWarby
  • 311
  • 3
  • 3
  • Pretty interesting syntax! – LarryBud Jul 01 '21 at 20:05
  • That's not ideal - discards are for when you want to throw away the value, but in this case, there is no value (null). Even if you argue that it is valid to throw away null, The use of the null coalescing operator here is non-idiomatic (assigning an exception is not possible [pretend that you didn't see the exception but a method call - this is unexpected behavior]). – ryanwebjackson Apr 25 '22 at 21:21
  • 1
    Probably a moot point now as I would favour vivek nuna's answer for .Net 6 onwards. (That answer needs some upvotes BTW) – AndyWarby May 04 '22 at 10:23
15

I prefer the explicit exception, for these reasons:

  • If the method has more than one SomeClass argument it gives you the opportunity to say which one it is (everything else is available in the call stack).
  • What if you do something that may have a side effect before referencing x?
Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
12

I agree with the idea of failing fast - however it is wise to know why failing fast is practical. Consider this example:

void someMethod(SomeClass x)
{       
    x.Property.doSomething();
}

If you rely on the NullReferenceException to tell you that something was wrong, how will you know what was null? The stack trace will only give you a line number, not which reference was null. In this example x or x.Property could both have been null and without failing fast with aggressive checking beforehand, you will not know which it is.

Andrew Hare
  • 344,730
  • 71
  • 640
  • 635
12

I strongly agree with @tvanfosson. Adding to his answer, with .net 6 it’s very easy to throw the ArgumentNullException.

ArgumentNullException.ThrowIfNull(object);

Here’s the official documentation.

Vivek Nuna
  • 25,472
  • 25
  • 109
  • 197
9

I'd prefer the parameter check with the explicit ArgumentNullException, too.

Looking at the metadata:

 //
    // Summary:
    //     Initializes a new instance of the System.ArgumentNullException class with
    //     the name of the parameter that causes this exception.
    //
    // Parameters:
    //   paramName:
    //     The name of the parameter that caused the exception.
    public ArgumentNullException(string paramName);

You can see, that the string should be the name of the parameter, that is null, and so give the developer a hint on what is going wrong.

Oliver Friedrich
  • 9,018
  • 9
  • 42
  • 48
6

You should explicitly throw an ArgumentNullException if you are expecting the input to not be null. You might want to write a class called Guard that provides helper methods for this. So your code will be:

void someMethod(SomeClass x, SomeClass y)
{
    Guard.NotNull(x,"x","someMethod received a null x argument!");
    Guard.NotNull(y,"y","someMethod received a null y argument!");


    x.doSomething();
    y.doSomething();
}

The NonNull method would do the nullity check and throw a NullArgumentException with the error message specified in the call.

Szymon Rozga
  • 17,971
  • 7
  • 53
  • 66
  • 5
    How about the stacktrace in the exception? It would contain the Guard method NotNull, which is only noise, and might cause confusion. Is there some way of avoiding this? – khebbie Sep 26 '09 at 13:02
  • 1
    @khebbie `try { Guard.NotNull(...); ... } catch (Exception ex) { throw ex; }` Just be sure NOT to rethrow it using `throw;` with no arguments, or it won't have its stack trace overwritten. – Zenexer Apr 16 '13 at 19:09
  • +1 I like that "Guard" approach. The better code readability outweights the little noise in the stacktrace imo. I'd even make guard a subclass of ArgumentNullException such as MyArgumentException.ThrowIfNull(x,"x","received a null x argument!") – Sebastian Feb 17 '15 at 07:43
  • Well it's a bit of extra work, but you [can construct the stacktrace yourself and tell the stacktrace constructor to skip one frame](https://msdn.microsoft.com/en-us/library/wybch8k4%28v=vs.110%29.aspx) so that the Guard.NotNull gets excluded from the trace. You of course have to inherit from your desired exception type as well and override the [Exception.StackTrace](https://msdn.microsoft.com/en-us/library/system.exception.stacktrace%28v=vs.110%29.aspx) property. Hmm... actually quite tempted to make a small lib for it. Well anyway you decide if it's worth it or not - it's just code-candy. – AnorZaken Sep 23 '15 at 18:58
  • If you use Code Analysis, you'll have to suppress rule [CA1062: Validate arguments of public methods](https://msdn.microsoft.com/en-us/library/ms182182.aspx). That is because Code Analysis won't know that `Guard.NotNull` is doing the check for `null`. – DavidRR Apr 05 '16 at 15:27
5

It is better to throw the ArgumentNullException sooner rather than later. If you throw it, you can provide more helpful information on the problem than a NullReferenceException.

g .
  • 8,110
  • 5
  • 38
  • 48
4
  1. Do it explicitly if you do not want a Null value. Otherwise, when someone else look at your code, they will think that passing a Null value is acceptable.

  2. Do it as early as you can. This way, you do not propagate the "wrong" behavior of having a Null when it's not supposed to.

Pang
  • 9,564
  • 146
  • 81
  • 122
Patrick Desjardins
  • 136,852
  • 88
  • 292
  • 341
4

Since .NET 6 you can throw the argument null exception in one line for instance:

int? foo = null;
ArgumentNullException.ThrowIfNull(foo);

This will check if foo is null and throw an error. You can pass a second parameter to set the parameter name, but this is not recommended.

https://learn.microsoft.com/en-us/dotnet/api/system.argumentnullexception.throwifnull?view=net-6.0

Tasty213
  • 395
  • 2
  • 10
  • 1
    You don't need to pass the second argument. *"The `paramName` parameter is included to support the `CallerArgumentExpressionAttribute` attribute. It's recommended that you don't pass a value for this parameter and let the name of argument be used instead."* – Theodor Zoulias Jun 01 '22 at 08:57
3

If you program defensively you should fail fast. So check your inputs and error out at the beginning of your code. You should be nice to your caller and give them the most descriptive error message you can.

Aaron Fischer
  • 20,853
  • 18
  • 75
  • 116
3

I'll probably be downvoted for this, but I think completely different.

What about following a good practice called "never pass null" and remove the ugly exception checking?

If the parameter is an object, DO NOT PASS NULL. Also, DO NOT RETURN NULL. You can even use the Null object pattern to help with that.

If it's optional, use default values (if your language supports them) or create an overload.

Much cleaner than ugly exceptions.

andrecarlucci
  • 5,927
  • 7
  • 52
  • 58
  • 2
    You don't know who might be calling your method some day in the future long after you changed jobs, or how many layers an object has passed through before reaching your method. Relying on your method not receiving null is not a viable option as it's beyond your control. Throwing a meaningful exception is within your control. – Søren Ullidtz Nov 07 '16 at 04:24
  • "You don't know who might be calling your method some day in the future long after you changed jobs" <- then you have a communication problem, not a code problem. The less code, the better. For me, using argument checking brings benefit only when you need to fail fast. Otherwise, don't pass null. And btw, there will be an exception somewhere anyway. – andrecarlucci Dec 28 '16 at 16:58
  • 2
    I would say you always need to fail fast, and make sure exceptions are meaningful. I suppose if no third parties have to use your code and you work on a small team in the same room you can be more lax about error handling. But that's rather circumstantial. In any case I think this is more of a topic for meta. – Søren Ullidtz Dec 29 '16 at 05:30
2

All code examples use error prone IF clause, where one use equal operator ==.

What happen, if type override == ?

On C# 7 and greater, use constant pattern matching.

example:

if (something is null) 
{
    throw new ArgumentNullException(nameof(something), "Can't be null.");
}
Frederik Hoeft
  • 1,177
  • 1
  • 13
  • 37
1

You can use syntax like the following to not just throw an ArgumentNullException but have that exception name the parameter as part of its error text as well. E.g.;

void SomeMethod(SomeObject someObject)
{
    Throw.IfArgNull(() => someObject);
    //... do more stuff
}

The class used to raise the exception is;

public static class Throw
{
    public static void IfArgNull<T>(Expression<Func<T>> arg)
    {
        if (arg == null)
        {
            throw new ArgumentNullException(nameof(arg), "There is no expression with which to test the object's value.");
        }

        // get the variable name of the argument
        MemberExpression metaData = arg.Body as MemberExpression;
        if (metaData == null)
        {
            throw new ArgumentException("Unable to retrieve the name of the object being tested.", nameof(arg));
        }

        // can the data type be null at all
        string argName = metaData.Member.Name;
        Type type = typeof(T);
        if (type.IsValueType && Nullable.GetUnderlyingType(type) == null)
        {
            throw new ArgumentException("The expression does not specify a nullible type.", argName);
        }

        // get the value and check for null
        if (arg.Compile()() == null)
        {
            throw new ArgumentNullException(argName);
        }
    }
}
DiskJunky
  • 4,750
  • 3
  • 37
  • 66
  • 2
    This solution creates a **considerable runtime overhead**, so I cannot recommend it. – Marcel Nov 22 '17 at 12:21
  • 1
    @Marcel, "considerable" is relative. For most business/domain layer situations, it's fine - there's no noticeable lag (sub-millisecond). For a high performance piece of code? Try and avoid `try/catch` altogether, as it also has very high runtime overhead. It's a case of use what you need, where you need it. – DiskJunky Dec 01 '17 at 19:48
  • @DiskJunkey: I'm not talking about the exception part. This is just fine. In most implementations try/catch does not cause a significant runtime overhead unless an exception is actually thrown. The expensive part is `Expression>` which causes the runtime code to create a new expression tree before each invocation of `IfArgNull`, even if everything is fine and the argument is not null. – Marcel Dec 02 '17 at 14:41
  • 1
    @Marcel, you're correct that the call is comparatively expensive but given the increase in readability I think the minor performance hit is negligible in practice. `Expression>` is used liberally with LINQ for example. Performance hit, yes, but a minor one overall. In this case, readability and maintainability trumps technical performance. That's just me though :-) – DiskJunky Dec 11 '17 at 14:53
  • maybe im late but ive been using a similar check code from year 2014 in legacy apps (when nameof was not available) and my last tests with about 100.000 checks took 19 seconds vs 0,2 seconds with a traditionals ifs. So I had to remove it. – cpsaez Apr 06 '18 at 10:48
  • @cpsaez, it's always a judgement call. If in a high-performance piece of code, of course avoid lambdas, anonymous delegates, etc. However, if it's a piece of code that gets hit once a second...it's not much (if at all), an issue. As with all tools; use judiciously. – DiskJunky Apr 08 '18 at 19:48
  • 1
    LINQ uses Expresssions only when using IQueryable for generating SQL queries, and it (usually) does not compile and execute them, rather it just parses them. The cost of putting together a SQL query and sending it to a database is high enough that this usually doesn't have a significant impact. The cost of simply null checking a parameter v.s. using this method you've presented is crazy to use, even for non-performance sensitive code. – Mike Marynowski May 11 '21 at 16:19