Why is this extension method not working?

Question

I can't get to do what I want. I only want accounts for non-international representatives. But when I call ActiveAccounts(), I'm not getting null, I'm getting an enumerable, which then includes null. What am I doing wrong here? Help please.

public static class AccountExt
{
    public static IEnumerable<Account> ActiveAccounts( this AccountRep rep )
    {
        if( rep == null )
            throw new ArgumentNullException();
        if( rep.IsInternational )
            yield return null;

        foreach( var acc in rep.FetchAccounts() )
        {
            if( acc.IsActive )
                yield return acc;
        }
    }
}

Answer 1

Well, there's a couple of things going on here.

First, you don't just have an extension method, you have an extension method iterator block - that's what you get when you use yield return to automatically implement the IEnumerable<> contract.

It sounds like what you want to have happen is for ActiveAccounts() to return a null IEnumerable<Account>. What's actually happening is that for international reps you are returning null as the first element of the IEnumerable. I suspect that you may have tried using return null there but got a compiler error something like:

Error: Cannot return a value from an iterator. Use the yield return statement to return a value, or yield break to end the iteration.

If what you intended is for the enumerable to be empty, what you want is yield break instead of yield return null. It's actually a better idea, generally, to return an empty sequence, as it allows the caller to avoid checking the return value. It also player nicer with technologies like LINQ, which use composition to assemble complex queries.

The second issue is that the if( rep == null ) precondition is not evaluated when you call ActiveAccounts(), but rather when you begin to enumerate the result of that call. That's probably not what you want - I'd imagine you want the precondition evaluated immediately.

The way you resolve both these problems is to use a two-stage implementation:

public static class AccountExt
{
   // apply preconditions, return null for international reps
   public static IEnumerable<Account> ActiveAccounts( this AccountRep rep )
   {
       if( rep == null )
           throw new ArgumentNullException( "rep" );
       if( rep.IsInternational )
           return null;
       // otherwise...
       return ActiveAccountsImpl( rep );
   }

   // private implementation handles returning active accounts
   private static IEnumerable<Account> ActiveAccountsImpl( AccountRep rep )
   {
       foreach( acc in rep.FetchAccounts() )
       {
           if( acc.IsActive )
               yield return acc;
       }
   }
}

If you're willing to use LINQ, to can avoid the Impl version of the function:

   public static IEnumerable<Account> ActiveAccounts( this AccountRep rep )
   {
       if( rep == null )
           throw new ArgumentNullException( "rep" );
       if( rep.IsInternational )
           return null;
       // otherwise, using LINQ to filter the accounts...
       return rep.FetchAccounts().Where( acc => acc.IsActive );
   }

You can learn more about how iterator blocks here.

Answer 2

You should replace yield return null with yield break. That will give back an empty sequence.

If you really want to return null instead of an IEnumerable then LBushkin's answer is the one you want; however, it's more common practice to return an empty sequence, as it doesn't require the consumer to have to check the return value.