Is this a safe fix for "Possible Multiple enumeration of IEnumerable"

Question

I have this currently:

public void Load(IEnumerable<Guid> ids){
    f1(ids);
    f2(ids);
}

To avoid Resharper squiggles, the suggested fix is something like

public void Load(IEnumerable<Guid> ids){
    var enumerable = ids as IList<Guid> ?? ids.ToList();
    f1(enumerable);
    f2(enumerable);
}

The problem is I can never think of a good name for "enumerable", I don't want to call it idsList, or enumeratedIds, and enumerable certainly won't work, and ids is already used for the parameter. So my actual question is will this solve both problems?

public void Load(IEnumerable<Guid> ids){
    ids = ids as IList<Guid> ?? ids.ToList();
    f1(ids);
    f2(ids);
}

Are there situations where the above would yield unexpected/inefficient results?

You are worrying about variable names with method names like `f1`, `f2`? — Tim Schmelter, Sep 23 '13 at 21:29
Looks like a duplicate of this issue: [Handling warning for possible multiple enumeration of IEnumerable][1] [1]: http://stackoverflow.com/questions/8240844/handling-warning-for-possible-multiple-enumeration-of-ienumerable — harley.333, Sep 23 '13 at 21:32
There will be if you try to update/delete the collection with something while it is trying to enumerate to the next one. — 123 456 789 0, Sep 23 '13 at 21:32
@TimSchmelter ironically, I couldn't think of any good example function names either. I guess that's one of my weaknesses. — Eric B, Sep 23 '13 at 21:38
If you want a list, take a list, period. If you're going to cast/convert anyway why even bother taking an IEnumerable to begin with? — Ed S., Sep 23 '13 at 21:39

Sergey Kalinichenko · Accepted Answer · 2013-09-23T21:39:46.943

Are there situations where the above would yield unexpected/inefficient results?

In cases when neither f1 nor f2 enumerate the entire list of Guids, and the ids are not given to you as a List<Guid>, converting to list would be more expensive. For example, when f1 takes the first five elements, f2 takes FirstOrDefault, ids come from a non-cached context, and contain thousands of items, the ToList approach is going to be less efficient.

To avoid Resharper squiggles, the suggested fix is ...

Not all Resharper warnings must be fixed. The tool attracts your attention to the problem, and it is up to you to fix it or to ignore the warning. Resharper is consistent at looking out for potential trouble, but only you know what's happening inside f1 and f2. If you think that it's more efficient to avoid materializing the list, use the // ReSharper disable comments to avoid the squiggles.

score 2 · Answer 2 · answered Sep 23 '13 at 21:42

If you are working with Entity Framework where you have execution-deferred collections of entities, this could be a very bad thing to do! Imagine if your ids IEnumerable was really defined by something like this:

var ids = MyDbContext.BillionsOfAccounts.Select(x => x.id);

and your functions were something like this (this may not compile but gets the point across):

public IEnumerable<T> f1(IEnumerable<T> query)
{
  if (query is IQueryable<T>)
  {
    return query.Take(10).ToList();
  }
  return null;
}

public IEnumerable<T> f2(IEnumerable<T> query)
{
  if (query is IQueryable<T>)
  {
    return query.Where(id => (id % 100000000) == 0).ToList();
  }
  return null;
}

If this were the case, then what you're doing could potentially attempt to fetch billions of records from a database instead of the 10 that were intended.

That said, there are many cases where what you're attempting is what you want. As others have said, you'll have to handle this on a case-by-case basis. But to answer your question, yes, there are scenarios where this could be very bad!

Is this a safe fix for "Possible Multiple enumeration of IEnumerable"

2 Answers2