Support ArgumentException for guard clause

[zvirja]

Closes #1107

It's fully legitimate when base ArgumentException is thrown instead of ArgumentNullException if null value is specified. It's very often case in case of string type, when you handle null and empty at the same time.

Additionally I've slightly re-organized facade tests for the GuardClauseAssertion as file was really huge.

@moodmosaic Please take a look. Change should be OK, but if you have time to give it a brief look - it would be awesome 😉

[moodmosaic]

👍 for the housekeeping. I'm concerned about catching ArgumentException instead of ArgumentNullException in that particular place. See comments below.

[moodmosaic]

This is a breaking change. See The IsNullOrWhiteSpace trap article by @ploeh.

We should need another guard clause assertion to deal with empty and white space strings ("", " ", etc).

[zvirja]

Thanks for the review.

I honestly don't understand why it's a breaking change (well, it's a change, but don't necessarily BREAKING). The article you pointed to describes that you shouldn't blindly assert string.IsNullOrWhiteSpace, as string with whitespaces might be still a valid thing.

However, if you have a guard clause for null, why is it so important to throw ArgumentNullException, but not the ArgumentException? Anyway you are complaining about wrong argument and anyway you are using the ArgumentException family - I don't see too much harm with that. I find ArgumentNullException as just one of the ways of complaining about that, but not the only way.

Notice, I'm not handling empty or whitespace strings here somehow. I just mentioned that sometimes due to a way you write the code the current restriction of having specifically ArgumentNullException causes pain.

@ploeh Could you please share your opinion also?

[ploeh]

I'm not sure that it's a breaking change in the sense that this change will break existing clients (including tests), but the documentation states:

"when an action (such as a method call or constructor invocation) is performed with a null argument it should raise an ArgumentNullException."

In that sense, this is a breaking change, so I would discourage this edit. I think that @zvirja himself provides the best argument against this change:

you are using the ArgumentException family

Indeed, and you could expand that argument to: you are using the Exception family. Why not simply catch (Exception e)?

why is it so important to throw ArgumentNullException, but not the ArgumentException?

Its Postel's law applied:

Be conservative in what you send, be liberal in what you accept.

Thrown exceptions are a kind of output, so it follows Postel's law to be as specific about it as possible.

What would be the reason/motivation for throwing an ArgumentException when you can throw ArgumentNullException instead?

[zvirja]

Indeed, and you could expand that argument to: you are using the Exception family. Why not simply catch (Exception e)?

I see your point, but I feel you are playing with words a bit and the common sense should apply 😉 Saying that ArgumentNullException belongs to the ArgumentException family makes way more sense than saying that ArgumentNullException belongs to the Exception family.

Otherwise if we start to go extreme, the opposite applies as well: if we want to follow the Postel's law, then instead of throwing generic ArgumentNullException we should throw more specific FilePathArgumentNullException, OutputStreamArgumentNullException and so on. But we don't do it, as it doesn't make any practical sense, so we generalize all the null arg cases to a generic exception.

So I'm not sold I proposed something which makes no sense 🤔

What would be the reason/motivation for throwing an ArgumentException when you can throw ArgumentNullException instead?

As I previously said, it mostly happens for string arguments, when I assert together that argument shouldn't be neither null nor empty. See below.

---

Let's try to think together what is the alternative solution.

Imagine, you have code like this:

public string MakeFullName(string firstName, string secondName)
{
    if (string.IsNullOrEmpty(firstName))
        throw new ArgumentException("Value cannot be null or empty.", nameof(firstName));
    if (string.IsNullOrEmpty(secondName))
        throw new ArgumentException("Value cannot be null or empty.", nameof(secondName));

    return firstName + " " + secondName;
}

It's pretty naive function, but this pattern happens very often - you don't allow neither null nor empty string.

Currently, in order to satisfy GuardClauseAssertion you would have to write 2 separate assertions per argument:

public string MakeFullName(string firstName, string secondName)
{
    if (firstName == null) throw new ArgumentNullException(nameof(firstName));
    if (firstName.Length == 0) throw new ArgumentException("Value cannot empty.", nameof(firstName));
    if (secondName == null) throw new ArgumentNullException(nameof(secondName));
    if (secondName.Length == 0) throw new ArgumentException("Value cannot empty.", nameof(secondName));

    return firstName + " " + secondName;
}

Even though observable result will be nicer, we are always redundant to pollute code with that as it's pure overkill.

Now let's take a look at GuardClauseAssertion and how you configure it:

var assertion = new GuardClauseAssertion(
    new Fixture(),
    new CompositeBehaviorExpectation(
        new NullReferenceBehaviorExpectation(),
        new EmptyGuidBehaviorExpectation()));

The expectations you pass are always probed and some of them might decide to not kick in if not relevant (like Guid exception happens only for Guid parameters). The NullReferenceBehaviorExpectation kicks in for all the reference types. Therefore, if you want to verify that bothempty and null strings are not allowed, you configure assertion like this:

var assertion = new GuardClauseAssertion(
    new Fixture(),
    new CompositeBehaviorExpectation(
        new NullReferenceBehaviorExpectation(),
        new EmptyGuidBehaviorExpectation()
        new EmptyStringBehaviorExpectation())); // checks only empty strings

As result, for string argument two assertions will be executed:

• NullReferenceBehaviorExpectation
• EmptyStringBehaviorExpectation

And the first one will fail, as exception is not ArgumentNullException, but the ArgumentException.

I cannot also fix it by having config like this:

var assertion = new GuardClauseAssertion(
    new Fixture(),
    new CompositeBehaviorExpectation(
        new EmptyGuidBehaviorExpectation()
        new NullOrEmptyStringBehaviorExpectation())); // checks only empty strings

as I might have non-string arguments as well and those should be also verified.

---

Given all that context, I thought that the nicest solution to the problem is to loose up the NullReferenceBehaviorExpectation to allow ArgumentException as well. I still solely believe that the following two lines are equivalent from the client perspective:

• new ArgumentNullException(nameof(arg))
• new ArgumentException("Value should not be null", nameof(arg))

Both of them clearly indicate that issue happened with argument and that input was wrong. Both of them carry enough information to understand why this exceptional situation happened. Yes, you might be more specific and throw more concrete exception instead, but if you had reasons to not do it then it's still OK. And yes, lets not use arguments like "why don't throw then new Exception($"Parameter {nameof(arg)} should not be null") instead?" to criticize this idea, as it's not the same. Instead if there are other more practical reasons - I would be happy to hear and consider them.

As a bottom line, I want to emphasize that I'm flexible and open to the alternative approaches. So if you see better way to solve it with the current architecture - please share it 🙏

[ploeh]

This is a deep discussion that we're unlikely to resolve in a few exchanges, but I'll try to expand on my position. The bottom line, however, is that I'm no longer in charge of this project, so do whatever you consider best.

On exception sub-typing

For a time being, I'd like you to forget about empty strings. We're going to have to talk about exception types first.

lets not use arguments like "why don't throw then new Exception($"Parameter {nameof(arg)} should not be null") instead?"

Sorry, that's not how discussions work 😉

That is, indeed, the fundamental question.

Why do we ever throw exceptions of particular sub-types, instead of just sticking with Exception?

Some of it is because of coding conventions, but technically, I'm only aware of one reason: it's because we might want to catch a particular sub-type, instead of all exceptions. This is sometimes necessary, but I can't think of any example where it's not, fundamentally, a design smell.

Even if we grant that one may occasionally need to catch particular exception sub-types, do you ever catch ArgumentException? Do you ever catch ArgumentNullException?

Those sub-types are, in my experience, never caught. What happens to uncaught exceptions? You may log them, but after that, you let the application crash.

Why do you need sub-typing for that?

You don't. You can log any type of exception, and when you read the log, if the exception message is sufficiently informative, then what does the sub-type tell you?

As far as I can tell, it's only because of conventions that we throw ArgumentException or ArgumentNullException. If there's any technical reason for it, I'd like to hear about it.

On empty strings

Let's return to empty strings. Why do you want to throw exceptions when strings are empty?

My code rarely throws exceptions for empty strings.

Instead of:

public string MakeFullName(string firstName, string secondName)
{
    if (firstName == null) throw new ArgumentNullException(nameof(firstName));
    if (firstName.Length == 0) throw new ArgumentException("Value cannot empty.", nameof(firstName));
    if (secondName == null) throw new ArgumentNullException(nameof(secondName));
    if (secondName.Length == 0) throw new ArgumentException("Value cannot empty.", nameof(secondName));

    return firstName + " " + secondName;
}

why don't you just write the following?

public string MakeFullName(string firstName, string secondName)
{
    if (firstName == null) throw new ArgumentNullException(nameof(firstName));
    if (secondName == null) throw new ArgumentNullException(nameof(secondName));

    return firstName + " " + secondName;
}

Doesn't this work just as well?

(In fact, for string concatenation, you can actually dispense with the null check as well...)

Why do you need the guard against empty strings? Most of the operations you can perform on strings work just fine on empty strings as well.

• You can reverse an empty string
• You can convert an empty string to upper- or lowercase
• You can split an empty string over a delimiter
• You can concatenate empty strings
• You can search the empty string for substrings
• You can enumerate the characters in the empty string
• You can measure the length of the empty string
• You can return all the distinct characters in the empty string
• You can sort the contents of the empty string

... I could go on.

There's a few things that you can't do with an empty string, most notably find the first or last character, but you can do that with a string with at least one character (including white-space characters).

So unless you specifically need the first or last character (or a few other pieces of information), then why guard against empty strings?

Why do I bring this up? Because it seems to me that the argument hinges on it being 'inconvenient' to have to write two guard clauses when 'one might do'. Furthermore, the argument is particularly tied to strings. It doesn't apply in general.

I question the premise of the argument. I don't see it as a general problem that one has to write both types of guards.

Summary

We throw specific exception types by convention, more than for any technical reason. That's okay, but then I think that we might as well follow Postel's law and be as specific as we can.

In general, I see few reasons to even guard against empty or white-space strings, so I consider it a non-issue to have to distinguish between null and empty strings.

Null references are evil; empty strings are typically entirely benign.

[zvirja]

UPD Moved maintenance commit to a separate PR and merged it there.