Throwing an exception for errors that can be fixed

Question

Say I have a class like this:

public class MyObject
{
    public List<string> MyCollection { get; set; }
}

And a method like this:

public void DoSomething(MyObject object)
{
    if(object.MyCollection == null)
    {
        // MyCollection must not be null
        // Should I...

        // a)
        object.MyCollection = new List<string>();

        // b)
        throw new ArgumentException("MyCollection can not be null");
    }
}

I do not have control over MyObject. Normally I'd just instantiate the collection in the constructor and be done with it. Should I just instantiate the collection in my method, or throw an exception?

Answer 1

What you've got here are called guard statements, and you absolutely should throw an exception if object.MyCollection is null.

Exceptions are meant for exceptional circumstances, and since you specify that object.MyCollection must not be null this would be indeed exceptional. Just make sure the exception you throw is of a suitable type (for example an ArgumentNullException).

Answer 2

Definitely B, it would be a violation of Single Responsibility Principle to do A since that's really not the purpose of that block of code.

my personal preference is only to do validation like that however on boundary methods; like web service endpoints or general API fascia of some nature, after that I find maintainability improved when the code let's improper usage just cause an error naturally (dereferencing for instance in your case), this way when what previously was invalid input becomes valid input, there's less code churn. Besides, an exception will be thrown if you use it, so why bother throwing your own exception?

This preference about often avoiding guard statements is mine and mine alone, take it with a grain of salt and only to mean that you should analyze your scenarios yourself to see what makes most sense to you, don't always use guard statements just because "best practices"

That said, for this particular case I tend to take advantage of the null coalesce operator; if I want to do something with a collection which I feel may be null for some reason, I'll instead deal with (thatCollection ?? new List<T>()) in place of it, so you end up treating a null list like an empty list as a part of your functions contract as opposed to meddling with somebody elses object which isn't your responsibility. This is only valid if you are not adding elements to the collection.

I take the approach I do just to be robust as it avoids future churn, though I make certain it's a valid behaviour within the functional requirements before I take that approach (it often is). Remember:

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

Answer 3

Guard conditions should, IMO, only be used when you won't or can't reverse a proces. Otherwise let the exception occur downstream where the value doesn't make sense.

Arguments to a function should meet the needs of the function, the only place that can guarantee that the correct values are provide is the calling site.

In your example, how would DoSomething know that an empty MyCollection will result in an acceptable action/value as a one with a dozen values in it.

Consider if MyCollection was BackupToBeforeDelete -- if you provide an empty list, the object doesn't get backed up, but does get deleted.