1

I'm new to C# and was practicing throwing Exceptions. Is it good practice to throw an exception from a helper method to shorten the amount of code needed? Like so:

    public static void ThrowExcIfNull<T>(T[] array)
    {
        if (array == null) throw new ArgumentNullException("Array is null");
    }
    
    /// <summary>
    /// Does Something
    /// </summary>
    /// <param name="x">The int array to be used</param>
    /// <exception cref="ArgumentNullException">Thrown when the string is 
    /// null</exception> //Is this correct?
    /// <returns>Some integer</returns>
    public static int SomeMethod(this int[] x)
    {
       ThrowExcIfNull(x);
       //Some code here
    }

Also, would it be okay to write the documentation saying that "the exception is being thrown from someMethod"? Any information would help! Thanks

MP1
  • 35
  • 1
  • 6

1 Answers1

5

I think you should use the following pattern instead:

using System;

public static class MyExtensions
{
    /// <summary>
    ///     Magic method.
    /// </summary>
    /// <param name="source">
    ///     The source array.
    /// </param>
    /// <exception cref="ArgumentNullException">
    ///     <paramref name="source" /> is <c>null</c>.
    /// </exception>
    /// <returns>
    ///     Some magic value only you know about.
    /// </returns>
    public static int SomeMethod(this int[] source)
    {
        if (source == null)
            throw new ArgumentNullException(nameof(source));

        return 42;
    }
}

Why?

  • you exposed ThrowExcIfNull as an extension method, that's quite weird to be honest
  • if you look at https://referencesource.microsoft.com/#q=throwif you'd see they're never public
  • except for CancellationToken.ThrowIfCancellationRequested but that's an exceptional situation

If you absolutely want such method

At least pass parameter name so it's easier to debug:

using System;

public static class MyExtensions
{
    public static int SomeMethod(this int[] source)
    {
        ThrowIfNull(source, nameof(source));

        return 42;
    }

    private static void ThrowIfNull(object value, string parameter)
    {
        if (value == null)
            throw new ArgumentNullException(parameter);
    }
}

But now you have another problem, the first method shown in stack trace will be ThrowExcIfNull:

enter image description here

Just look at the difference without using that helper method:

enter image description here

It's crystal clear where the error comes from.

You will probably want this method:

  • if you use it in literally hundreds of places
  • if messages are to be translated to user's culture, i.e. Chinese for instance
  • etc
aybe
  • 15,516
  • 9
  • 57
  • 105
  • Good point about it being an extension method. I missed that when I was writing my first comment. – madreflection Apr 03 '19 at 03:34
  • 3
    Additionally, putting the `throw` into a helper method helps the calling method to be inlined. In heavy hot paths this can increase performance. – yaakov Apr 03 '19 at 04:31
  • If there were two separate methods, Should I write the documentation tag on the calling method or the helper method or does it matter? Also +1 for the answer to life in the response. Thanks again! – MP1 Apr 03 '19 at 05:12
  • Lol, you should write documentation for public members, for private members unless it's really complex I generally don't bother, with good parameters names it's already a good documentation in itself – aybe Apr 03 '19 at 08:39
  • @aybe have you ever found a way to both use the helper method and emit a proper stack trace? (ie throw new [Type]Exception(message, StackFrames[-1]) Of note, its a very common use case in the framework libraries, since they needed to tailor the exception messages for a given language (which ironincally most of the time isnt the user's language but the language of the server itself) – enorl76 Dec 18 '21 at 19:41
  • Never really bothered, thing is, this patterns screws some analyzers and I tend to not use very often in the end.... – aybe Dec 19 '21 at 05:51