ArgumentException or ArgumentNullException for string parameters?

Question

Far as best practices are concerned, which is better:

public void SomeMethod(string str) 
{
    if(string.IsNullOrEmpty(str))
    {
        throw new ArgumentException("str cannot be null or empty.");
    }

    // do other stuff
}

or

public void SomeMethod(string str) 
{
    if(str == null) 
    {
        throw new ArgumentNullException("str");
    }

    if(str == string.Empty)
    {
        throw new ArgumentException("str cannot be empty.");
    }

    // do other stuff
}

The second version seems more precise, but also more cumbersome than the first. I usually go with #1, but figured I'd check if there's an argument to be made for #2.

Answer 1

I'd say the second way is indeed more precise - yes, it's more cumbersome but you can always wrap it in a method to avoid having to do it all the time. It could even be an extension method:

str.ThrowIfNullOrEmpty("str");


public static void ThrowIfNullOrEmpty(this string value, string name)
{
    if (value == null)
    {
        throw new ArgumentNullException(name);
    }
    if (value == "")
    {
        throw new ArgumentException("Argument must not be the empty string.",
                                    name);
    }
}

Another form which is potentially useful is one which returns the original string if everything is okay. You could write something like this:

public Person(string name)
{
    this.name = name.CheckNotEmpty();
}

Another option to consider is using Code Contracts as an alternative to throwing your own exceptions.

Answer 2

I would suggest using the first one. If your method doesn't expects null or empty string it really doesn't matter if null or empty was passed - important to report and error and that is what 1st variant does.

Answer 3

Throwing a null reference on an empty string can be highly confusing - the string's default value is null and would indicate a not initialized string whereas and empty string may constitute other problems.

I like the idea of Jon Skeet however i like to explicitly use throw, and not have a separate function.

Instead you could throw the following class:

 public class ArgumentNullOrEmptyException : ArgumentNullException
{
    #region Properties And Fields

    private static string DefaultMessage => $"A value cannot be null or empty{Environment.NewLine}";

    #endregion

    #region Construction and Destruction

    public ArgumentNullOrEmptyException()
        : base(DefaultMessage)
    {
    }

    public ArgumentNullOrEmptyException(string paramName)
        : base(paramName, 
                  $"{DefaultMessage}" +
                  $"Parameter name: {paramName}")
    {
    }

    public ArgumentNullOrEmptyException(string message, Exception innerException)
        : base($"{DefaultMessage}{message}", innerException)
    {
    }

    public ArgumentNullOrEmptyException(string paramName, string message)
        : base(paramName, 
              $"{DefaultMessage}" +
              $"Parameter name: {paramName}{Environment.NewLine}" +
              $"{message}")
    {
    }

    #endregion

    public static void ThrowOnNullOrEmpty(string paramName, string paramValue)
    {
        if(string.IsNullOrEmpty(paramValue))
            throw new ArgumentNullOrEmptyException(paramName);
    }
    public static void ThrowOnNullOrEmpty(string paramValue)
    {
        if (string.IsNullOrEmpty(paramValue))
            throw new ArgumentNullOrEmptyException();
    }


    public static void ThrowOnNullOrEmpty(string paramName, object paramValue)
    {
        //ThrowOnNullOrEmpty another object that could be 'empty' 

        throw new NotImplementedException();
    }
}



        private static void AFunctionWithAstringParameter(string inputString)
    {
        if(string.IsNullOrEmpty(inputString)) throw new ArgumentNullOrEmptyException(nameof(inputString));

        //Or ..
        ArgumentNullOrEmptyException.ThrowOnNullOrEmpty(nameof(inputString), inputString);
    }

Note that my derived type only calls the base exception class instead of overriding the message property. This is because the 'additional info' of the visual studio debugger lists the internal message - not the overridden type. This means that this is the only way to display the message nicely whilst debugging.