Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question

3 Answers 3

up vote 8 down vote accepted

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).

share|improve this answer
1  
I'd use Asserts for these kind of "contract" checks. Advantage of them is that they serve as the check during development and can be turned off for production code by a simple compiler switch. –  Marjan Venema Jul 24 '13 at 19:23
    
@MarjanVenema - Do you have an example of what you mean? Don't Asserts throw exceptions anyway? Or did you mean write unit tests around the code? –  billy.bob Jul 25 '13 at 8:53
    
The point is that you want the checks and the exceptions in debug builds where getting it right is the priority and you want to ensure that developers getting it wrong are warned. But you don't want if assigned checks all over your code because it leads to situations wheren the not assigned scenario isn't handled. You should throw an exception in that case, which is what you answered, but that often tends to get forgotten. Plus you don't want all these if's and/or "exception throws" in a release build where performance is the priority. Asserts are a nice way of handling both use cases. –  Marjan Venema Jul 25 '13 at 9:56
    
Oh and yes, I even have unit tests ensuring that the asserts get fired. May sound paranoid, but it ensures that my asserts don't disappear "by accident" when they are meant to guard my contract. And all code relies on them getting fired in debug builds. –  Marjan Venema Jul 25 '13 at 9:57
2  
I wouldn't want the error to be handled differently in dev and production. That seems like it would introduce some nightmare bugs that only occur in production. –  ConditionRacer Jul 25 '13 at 14:12

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

share|improve this answer
4  
Watch out though, Postels law, your last quote, has brought us a lot of pain and can backfire because tolerating dodgy input soon becomes expected behavior that then has to be preserved. –  whatsisname Jul 24 '13 at 17:08
    
@whatsisname True, more than anything here I'm advocating you analyze your beliefs on these things before just going one way or another. I definitely use guard statements for many scenarios, it just varies based on each case, and the liberal acceptance when it requires less code than the alternative can be good for longer term functionality; it's far more frequent to break down constraints your code has so it may be used for more things than it is to apply new constraints. –  Jimmy Hoffa Jul 24 '13 at 17:12

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.

share|improve this answer
    
I guess my issue with this is that it's much more difficult to provide useful exceptions if I just let the exception occur naturally downstream. If I know up front that I can't continue with a null collection, then why not just fail right there with a descriptive message? –  ConditionRacer Jul 25 '13 at 14:09
1  
@Justin984 remember whenever you think about "useful exceptions" you're implicitly saying the natural exception is "not useful", but for this to be actually true, you have to assess how you're going to handle the particular error scenario; if the error scenario will be handled identically, with exactly the same code with the natural exception as with the "useful exception", then you're not getting any benefit from the "useful exception" are you? But you do now have an added constraint on a function that you might have to dismantle later, and extra code that yields no benefit... –  Jimmy Hoffa Jul 25 '13 at 14:53
    
@Justin984: instead of failing right there, why not go to the calling sites and fix them so they don't send an incomplete object? If you can't do that (maybe it's a library that others use), let the caller handle it. As JimmyHoffa said, what difference does it make? Either the caller adds a catch for SpecialException in which case they can just as easily add one for NullWhateverException or even better yet fix it, or they don't in which case you're giving a pretty exception that doesn't change how they handle it. More work, no improvement.... –  jmoreno Jul 25 '13 at 15:39
    
...now I admit to creating a special exception when trying to identify a particular type of bad user input -- where the input consists of a list and a single element is bad. I catch the exception and then throw one saying what element was incorrect. Not providing the list would be a problem for the caller, but since it doesn't know how to deal with the list, it can't handle bad items. By rethrowing with a custom exception, I get the input either logged or reported to the user in useful manner. –  jmoreno Jul 25 '13 at 15:47

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.