5

Say we have a method that looks like this:

public IEnumerable<Dog> GrowAll(this IEnumerable<Puppy> puppies)
{
    if(subjects == null)
        throw new ArgumentNullException("subjects");

    foreach(var puppy in puppies)
        yield return puppy.Grow();
}

If I test that by doing this:

Puppy[] puppies = null;
Assert.Throws<ArgumentNullException>(() => puppies.GrowAll());

The test will fail saying that it

Expected: <System.ArgumentNullException>
But was: null

I can fix that by changing the test to

Puppy[] puppies = null;
Assert.Throws<ArgumentNullException>(() => puppies.GrowAll().ToArray());

Is this just how you would usually do it? Or is there a better way to write the test? Or maybe a better way to write the method itself?


Tried to do the same with the built-in Select method, and it failed even without a ToArray or anything like that, so apparently there is something you can do about it... I just don't know what :p

Svish
  • 152,914
  • 173
  • 462
  • 620

1 Answers1

3

The test is fine - your code isn't. You should make the code throw the exception as soon as it's called, by splitting the method in half:

public IEnumerable<Dog> GrowAll(this IEnumerable<Puppy> puppies)
{
    if(subjects == null)
        throw new ArgumentNullException("subjects");

    return GrowAllImpl(puppies);
}

private IEnumerable<Dog> GrowAllImpl(this IEnumerable<Puppy> puppies)
{
    foreach(var puppy in puppies)
        yield return puppy.Grow();
}
Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Aaaah, so that's why all your methods look like that in that library of yours... Makes sense! Not sure if I like it though... hehe. Must say I prefer to keep it in one method. But oh well. I suppose I will get used to it after a while :) – Svish Jan 11 '10 at 13:18
  • 1
    It's an unfortunate side-effect of iterator blocks. See http://msmvps.com/blogs/jon_skeet/archive/2008/03/02/c-4-idea-iterator-blocks-and-parameter-checking.aspx – Jon Skeet Jan 11 '10 at 13:19
  • Exactly. Yeah, I kind of understood why it happened. Just not how the recommended way to deal with it was :) – Svish Jan 11 '10 at 13:27