7

If I define an extension method such as this:

static public String ToTitleCase(this string instance, CultureInfo culture)
{
    if (instance == null)
        throw new NullReferenceException();

    if (culture == null)
        throw new ArgumentNullException("culture");

    return culture.TextInfo.ToTitleCase(instance);
}

Is it necessary for me to check the string instance for null and throw an null reference exception myself? Or does the CLR treat extension methods like instance methods in this case and handle the checking/throwing for me?

I know extension methods are just syntactic sugar for static methods, perhaps the C# compiler adds in the check at compile time? Please clarify :)

MattDavey
  • 8,897
  • 3
  • 31
  • 54
  • 2
    The duplicate question provides some nice reasons in favor of ANE vs. NRE and other details. –  Jan 25 '11 at 10:44

2 Answers2

35

No. You should never throw a NullReferenceException manually. It should only ever be thrown by the framework itself.

In this context, you should be throwing ArgumentNullException for both instance and culture:

static public String ToTitleCase(this string instance, CultureInfo culture)
{
    if (instance == null)
        throw new ArgumentNullException("instance");

    if (culture == null)
        throw new ArgumentNullException("culture");

   return culture.TextInfo.ToTitleCase(instance);
}

From the NullReferenceException documentation:

Note that applications throw the ArgumentNullException exception rather than the NullReferenceException exception discussed here.

LukeH
  • 263,068
  • 57
  • 365
  • 409
  • 5
    I don't think your "never" has enough emphasis. – R. Martinho Fernandes Jan 25 '11 at 10:23
  • Is there a particular reason for the "never"? What if the NRE isn't a result of an argument being `null`? (Different case, but why the "never"?) –  Jan 25 '11 at 10:26
  • @pst: I'm not sure I follow you. An `NRE` is reserved for when you try to dereference a null object ref. If you potentially want an `NRE` then just dereference without doing any prior checks and let the framework throw it for you (not that I'd recommend that approach). If you're throwing your own exceptions then you should treat `NRE` as off-limits. – LukeH Jan 25 '11 at 10:31
  • @Martinho: Yep. I started off with caps-bold-italic, but it looked a bit too insane! – LukeH Jan 25 '11 at 10:32
  • @LukeH I can accept that. Answer is just sort of lacking as to *why* an NRE shouldn't be manually thrown. The except from the documentation also fails :-) If there was a 'SHOULD' or 'MUST' in there somewhere... in this particular case, why not an `NRE` when `instance` is null? This would mimic a non-extension method better. Culture should definitely be an `ANE`. –  Jan 25 '11 at 10:33
  • 1
    In this particular example a null reference exception would never be thrown by the framework since the string is not dereferenced. I'm just wondering if throwing a NullReferenceException myself would be more consistent for people consuming this method, ie. String foo = null; foo = foo.SubString(0, 10); // NullReferenceException foo = foo.ToTitleCase(c); // ArgumentNullException... confusing? – MattDavey Jan 25 '11 at 10:37
  • 1
    @Matt: An extension method is still *just* a static method, and the `this` parameter is still *just* a parameter to that method. Throwing `NRE` rather than `ANE` would, in my opinion, be more confusing and less consistent; no `null` is being dereferenced, but you have passed a `null` argument. – LukeH Jan 25 '11 at 10:45
  • 2
    Maybe you should mention this doc: [Creating and Throwing Exceptions - C# Programming Guide | Microsoft Docs](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/exceptions/creating-and-throwing-exceptions#things-to-avoid-when-throwing-exceptions) – walterlv Jun 20 '19 at 11:36
1

It most certainly is not. However, "fail fast" and, what some people forget ... "fail helpfully". However, I believe the reasons for not throwing an ArgumentNullException (debate vs. NullReferenceException left to other posts) are limited and usually related to over-cleverness :-) One hypothetical use-case may be IsNullOrEmpty. As long as it actually serves a purpose and makes code cleaner: go for it.

There is no check from the CLR. As far as the runtime is concerned it just passed a (possibly null) argument to a static method. The rest is sugar -- and none of that sugar involves adding extra null checks :-)

Happy coding.