.NET 6 replace parameter checks with ArgumentNullException.ThrowIfNull();

Question

I'm upgrading our application to .NET 6 from .NET 3.1 Core and trying to use the new ArgumentNullException.ThrowIfNull();

But in our code, we do not only check for null but also for other things.

if (anyParam == null)
{
    throw new ArgumentNullException(nameof(anyParam));
}

if (string.IsNullOrEmpty(stringParam))
{
    throw new ArgumentException(nameof(stringParam));
}

if (intParam <= 0)
{
    throw new ArgumentException(nameof(intParam ));
}

if (listParam.Count == 0)
{
    throw new ArgumentException(nameof(listParam));
}

Since Rule CA2208 now wants to change all ArgumentException lines, which we have a lot of, this would be a lot of work.

I want to know is it only safe to replace pure null checks like in the first example, or what is it all checking for in case of different parameter types. Couldn't find the proper documentation.

I would like to replace as much as I can with:

ArgumentNullException.ThrowIfNull(anyParam);

Highly

Answer 1

You are mixing up two completely unrelated issues here:

The first one is replacing your very first check with ArgumentNullException.ThrowIfNull. Sure, that's a good idea to do! Starting with .NET 7, you will also be able to replace your second check with ArgumentException.ThrowIfNullOrEmpty.

The second issue is that your second, third and fourth checks are broken and should be fixed (this is what CA2208 is about): The first parameter of ArgumentNullException is paramName, but the first parameter of ArgumentException is not paramName, it's message.

For example, your line

if (intParam <= 0)
{
    throw new ArgumentException(nameof(intParam));
}

should actually be:

if (intParam <= 0)
{
    throw new ArgumentException(nameof(intParam) + " must be a positive integer.");
}

(Or, optionally:)

if (intParam <= 0)
{
    throw new ArgumentException(nameof(intParam) + " must be a positive integer.", 
                                nameof(intParam));
}

---

As suggested in the comments, ArgumentOutOfRangeException might be an even better option for this particular check. Note though, that, contrary to ArgumentException's constructor, ArgumentOutOfRangeException (like ArgumentNullException) takes the parameter name first and then (optionally) the message. I.e., you can use either

if (intParam <= 0)
{
    // yield a generic "out of range" exception message
    throw new ArgumentOutOfRangeException(nameof(intParam));
}

or

if (intParam <= 0)
{
    throw new ArgumentOutOfRangeException(nameof(intParam),
        nameof(intParam) + " must be a positive integer.");
}

Foregone

I would suggest throwing ArgumentOutOfRangeException for the int checks. – Slaver

ArgumentException.ThrowIfNullOrEmpty don't exist in .NET 6. It's available from .NET 7. – Rotorua

Out of curiosity, isn't a burden to handle so many exceptions on upper layers? Local handlers, global handlers, retries, then retries at retries, and what if we don't see the forest by the trees? Meaning we fix exception 1 but after the fix exception 2 happens. – Deflagrate

Wouldn't be better beside constructors (where null argument exception is accepted), to use model state inside service layer learn.microsoft.com/en-us/aspnet/mvc/overview/older-versions-1/… or maybe learn.microsoft.com/en-us/dotnet/api/… if we want to throw an exception to cover all failed validations? – Deflagrate

@Deflagrate isn't a burden to handle so many exceptions on upper layers There's no exception handling in any of the code posted in the question or the answer. And let's suppose we don't throw an exception in the method guards. What then? We just get a NullReferenceException a little later? Or worse, an incorrect result from a calculation which doesn't throw an exception at all? – Slaver

It just a question, handler or no handler, ask remains, isn't exception used as a expected behavior dangerous and hard to maintain and there's the trap of if exception is of type x do that, but what if the type x resulted from another place, not from where usually is expected to happen, what then? – Deflagrate

@MatthewWatson sorry, are 2 comments (second related to first), I not saying to remove arguments validation, but instead of an exception throwing to use a different pattern to handle arguments/model validation – Deflagrate

ok so I'll replace only the null checks and for other occurences I'll use the correct syntax for ArgumentException. Also found there's a fix available from the rule which I can apply to the whole Solution – Highly

@Deflagrate Ah OK, I see what you mean. I guess that depends on where the methods being validated are - are they low-level, or are they in the business model etc. – Slaver

@MatthewWatson there in the low level data handler. So no handling there. Only throwing – Highly

@MatthewWatson: Good idea with ArgumentOutOfRange exception. I've added a note. – Foregone