420

In my code I need to use an IEnumerable<> several times, resulting in the ReSharper error of "Possible multiple enumeration of IEnumerable".

Sample code:

public List<object> Foo(IEnumerable<object> objects)
{
    if (objects == null || !objects.Any())
        throw new ArgumentException();
        
    var firstObject = objects.First();
    var list = DoSomeThing(firstObject);        
    var secondList = DoSomeThingElse(objects);
    list.AddRange(secondList);
    
    return list;
}
  • I can change the objects parameter to be List and then avoid the possible multiple enumeration but then I don't get the highest object that I can handle.
  • Another thing that I can do is to convert the IEnumerable to List at the beginning of the method:

 public List<object> Foo(IEnumerable<object> objects)
 {
    var objectList = objects.ToList();
    // ...
 }

But this is just awkward.

What would you do in this scenario?

9 Answers 9

544

The problem with taking IEnumerable as a parameter is that it tells callers "I wish to enumerate this". It doesn't tell them how many times you wish to enumerate.

I can change the objects parameter to be List and then avoid the possible multiple enumeration but then I don't get the highest object that I can handle.

The goal of taking the highest object is noble, but it leaves room for too many assumptions. Do you really want someone to pass a LINQ to SQL query to this method, only for you to enumerate it twice (getting potentially different results each time?)

The semantic missing here is that a caller, who perhaps doesn't take time to read the details of the method, may assume you only iterate once - so they pass you an expensive object. Your method signature doesn't indicate either way.

By changing the method signature to IList/ICollection, you will at least make it clearer to the caller what your expectations are, and they can avoid costly mistakes.

Otherwise, most developers looking at the method might assume you only iterate once. If taking an IEnumerable is so important, you should consider doing the .ToList() at the start of the method.

It's a shame .NET doesn't have an interface that is IEnumerable + Count + Indexer, without Add/Remove etc. methods, which is what I suspect would solve this problem.

10
  • 41
    Does ReadOnlyCollection<T> meet your desired interface requirements? msdn.microsoft.com/en-us/library/ms132474.aspx Dec 12, 2012 at 19:46
  • 94
    @DanNeely I would suggest IReadOnlyCollection(T) (new with .net 4.5) as the best interface to convey the idea that it is an IEnumerable(T) which is intended to be enumerated multiple times. As this answer states, IEnumerable(T) itself is so generic that it may even refer to un-resetable content which cannot be enumerated over again without side effects. But IReadOnlyCollection(T) implies re-iterability.
    – binki
    Nov 11, 2013 at 17:21
  • 1
    If you do the .ToList() at the start of the method, you first have to check if the parameter is not null. That means you'll get two places where you throw an ArgumentException: if the parameter is null and if it doesn't have any items.
    – comecme
    Jan 29, 2015 at 14:58
  • I read this article on the semantics of IReadOnlyCollection<T> vs IEnumerable<T> and then posted a question about using IReadOnlyCollection<T> as a parameter, and in which cases. I think the conclusion I eventually came to is the logic to follow. That it should be used where enumeration is expected, not necessarily where there could be multiple enumeration.
    – Neo
    Dec 23, 2016 at 16:55
  • 2
    Why does taking IList over IEnumerable silence this analysis warning in Resharper? Usually the practice is to take the most open/generic type; so I don't see how IList fixes the potential multiple iterations issue. This whole warning just feels very confusing to me. Oct 20, 2020 at 15:36
35

If your data is always going to be repeatable, perhaps don't worry about it. However, you can unroll it too - this is especially useful if the incoming data could be large (for example, reading from disk/network):

if(objects == null) throw new ArgumentException();
using(var iter = objects.GetEnumerator()) {
    if(!iter.MoveNext()) throw new ArgumentException();

    var firstObject = iter.Current;
    var list = DoSomeThing(firstObject);  

    while(iter.MoveNext()) {
        list.Add(DoSomeThingElse(iter.Current));
    }
    return list;
}

Note I changed the semantic of DoSomethingElse a bit, but this is mainly to show unrolled usage. You could re-wrap the iterator, for example. You could make it an iterator block too, which could be nice; then there is no list - and you would yield return the items as you get them, rather than add to a list to be returned.

7
  • 5
    +1 Well that is a very nice code, but- I loose the prettiness and readability of the code.
    – gdoron
    Nov 23, 2011 at 10:58
  • 5
    @gdoron not everything is pretty ;p I don't the the above is unreadable, though. Especially if it is made into an iterator block - quite cute, IMO. Nov 23, 2011 at 11:01
  • 11
    @gdoron in the question you explicitly said you didn't really want to do that ;p Also, a ToList() approach may not suit if the data if very large (potentially, infinite - sequences do not need to be finite, but can still be iterated). Ultimately, I'm just presenting an option. Only you know the full context to see if it is warranted. If your data is repeatable, another valid option is: don't change anything! Ignore R# instead... it all depends on the context. Nov 23, 2011 at 11:47
  • 1
    I think Marc's solution is quite nice. 1. It is very clear how the 'list' is initialized, it is very clear how the list is iterated, and it is very clear which members of the list are operated on. Jun 4, 2012 at 16:59
  • 2
    @anatol IEnumerable<T> just means "a sequence"; if that sequence is coming from some in-memory list/array/etc, then sure: it'll probably be repeatable - however - it could be pulling data from a socket, or a PRNG, or a ton of other places; fundamentally, IEnumerable<T> is not guaranteed to be repeatable Jul 15, 2021 at 11:03
25

Using IReadOnlyCollection<T> or IReadOnlyList<T> in the method signature instead of IEnumerable<T>, has the advantage of making explicit that you might need to check the count before iterating, or to iterate multiple times for some other reason.

However they have a huge downside that will cause problems if you try to refactor your code to use interfaces, for instance to make it more testable and friendly to dynamic proxying. The key point is that IList<T> does not inherit from IReadOnlyList<T>, and similarly for other collections and their respective read-only interfaces. (In short, this is because .NET 4.5 wanted to keep ABI compatibility with earlier versions. But they didn't even take the opportunity to change that in .NET core.)

This means that if you get an IList<T> from some part of the program and want to pass it to another part that expects an IReadOnlyList<T>, you can't! You can however pass an IList<T> as an IEnumerable<T>.

In the end, IEnumerable<T> is the only read-only interface supported by all .NET collections including all collection interfaces. Any other alternative will come back to bite you as you realize that you locked yourself out from some architecture choices. So I think it's the proper type to use in function signatures to express that you just want a read-only collection.

(Note that you can always write a IReadOnlyList<T> ToReadOnly<T>(this IList<T> list) extension method that simply casts if the underlying type supports both interfaces, but you have to add it manually everywhere when refactoring, where as IEnumerable<T> is always compatible.)

As always this is not an absolute, if you're writing database-heavy code where accidental multiple enumeration would be a disaster, you might prefer a different trade-off.

1
  • 9
    +1 For coming on an old post and adding documentation on new ways to solve this issue with the new features provided by the .NET platform (i.e. IReadOnlyCollection<T>) Sep 17, 2018 at 16:18
11

With .NET 6/ C# 10

.. and beyond you can attempt to determine the number of elements in a sequence without forcing an enumeration using Enumerable.TryGetNonEnumeratedCount(IEnumerable, Int32) method.

This method returns true if the count of source can be determined without enumeration; otherwise, false. So you can check if further implementation is needed.

using System;
using System.Collections.Generic;
using System.Linq;
                    
public class Program
{
    public static void Main()
    {
        IEnumerable<int> arrayOne = new[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

        var canGetCountDirectly = arrayOne.TryGetNonEnumeratedCount(out int theCount);

        Console.WriteLine($"Count can be returned directly = {canGetCountDirectly}");
        Console.WriteLine($"Count = {theCount}");
    }
}
2

If the aim is really to prevent multiple enumerations than the answer by Marc Gravell is the one to read, but maintaining the same semantics you could simple remove the redundant Any and First calls and go with:

public List<object> Foo(IEnumerable<object> objects)
{
    if (objects == null)
        throw new ArgumentNullException("objects");

    var first = objects.FirstOrDefault();

    if (first == null)
        throw new ArgumentException(
            "Empty enumerable not supported.", 
            "objects");

    var list = DoSomeThing(first);  

    var secondList = DoSomeThingElse(objects);

    list.AddRange(secondList);

    return list;
}

Note, that this assumes that you IEnumerable is not generic or at least is constrained to be a reference type.

5
  • 3
    objects is still being passed to DoSomethingElse which is returning a list, so the possibility that you are still enumerating twice is still there. Either way if the collection was a LINQ to SQL query, you hit the DB twice. Nov 23, 2011 at 11:04
  • 1
    @PaulStovell, Read this: "If the aim is really to prevent multiple enumerations than the answer by Marc Gravell is the one to read" =)
    – gdoron
    Nov 23, 2011 at 11:06
  • It has been suggested on some other stackoverflow posts on throwing exceptions for empty lists and I'll suggest it here: why throw an exception on an empty list? Get an empty list, give an empty list. As a paradigm, that's pretty straightforward. If the caller doesn't know they are passing an empty list in, that's there problem. Jun 4, 2012 at 17:02
  • @Keith Hoffman, the sample code maintains the same behavior as the code the OP posted, where the use of First will throw an exception for an empty list. I don't think its possible to say never throw exception on empty list or always throw exception on empty list. It depends... Jun 4, 2012 at 17:15
  • Fair enough Joao. More of food for thought than a criticism of your post. Jun 14, 2012 at 22:33
2

I usually overload my method with IEnumerable and IList in this situation.

public static IEnumerable<T> Method<T>( this IList<T> source ){... }

public static IEnumerable<T> Method<T>( this IEnumerable<T> source )
{
    /*input checks on source parameter here*/
    return Method( source.ToList() );
}

I take care to explain in the summary comments of the methods that calling IEnumerable will perform a .ToList().

The programmer can choose to .ToList() at a higher level if multiple operations are being concatenated and then call the IList overload or let my IEnumerable overload take care of that.

4
  • 26
    This is horrific. Oct 11, 2017 at 10:56
  • 1
    @MikeChamberlain Yeah, it really is horrific and utterly clean at the same time. Unluckily some heavy scenarios need this approach. Oct 11, 2017 at 13:05
  • 1
    But then you would be recreating the array each time you pass it to the IEnumerable parameratized method. I agree with @MikeChamberlain in this regard, this is a terrible suggestion.
    – Krythic
    Nov 21, 2017 at 23:48
  • 2
    @Krythic If you don't need the list to be recreated, you perform a ToList somewhere else and use the IList overload. That's the point of this solution. Nov 22, 2017 at 8:16
1

If you only need to check the first element you can peek on it without iterating the whole collection:

public List<object> Foo(IEnumerable<object> objects)
{
    object firstObject;
    if (objects == null || !TryPeek(ref objects, out firstObject))
        throw new ArgumentException();

    var list = DoSomeThing(firstObject);
    var secondList = DoSomeThingElse(objects);
    list.AddRange(secondList);

    return list;
}

public static bool TryPeek<T>(ref IEnumerable<T> source, out T first)
{
    if (source == null)
        throw new ArgumentNullException(nameof(source));

    IEnumerator<T> enumerator = source.GetEnumerator();
    if (!enumerator.MoveNext())
    {
        first = default(T);
        source = Enumerable.Empty<T>();
        return false;
    }

    first = enumerator.Current;
    T firstElement = first;
    source = Iterate();
    return true;

    IEnumerable<T> Iterate()
    {
        yield return firstElement;
        using (enumerator)
        {
            while (enumerator.MoveNext())
            {
                yield return enumerator.Current;
            }
        }
    }
}
2
  • Very nice solution! But you did not call Dispose in the case where the source is empty. Sep 23, 2022 at 10:04
  • Furhermore there is no Dispose when there is no iteration over the "new" source. Sep 23, 2022 at 10:50
0

I like the approach of methods taking a IReadOnlyCollection if they may need to enumerate the thing multiple times. I would like to offer a utility method that can be called whenever to turn an IEnumerable into a IReadOnlyCollection. Unlike ToArray() or ToList() it can avoid the cost of extra allocations changing the type of the underlying collection:

    public static class EnumerableUtils
    {
        /* Ensures an IEnumerable is only enumerated once  */
        public static IReadOnlyCollection<T> AsEnumerated<T>(this IEnumerable<T> original)
        {
            if (original is IReadOnlyCollection<T> originalCollection)
            {
                return originalCollection;
            }

            return ImmutableArray.CreateRange(original);
        }
    }
-3

First off, that warning does not always mean so much. I usually disabled it after making sure it's not a performance bottle neck. It just means the IEnumerable is evaluated twice, wich is usually not a problem unless the evaluation itself takes a long time. Even if it does take a long time, in this case your only using one element the first time around.

In this scenario you could also exploit the powerful linq extension methods even more.

var firstObject = objects.First();
return DoSomeThing(firstObject).Concat(DoSomeThingElse(objects).ToList();

It is possible to only evaluate the IEnumerable once in this case with some hassle, but profile first and see if it's really a problem.

7
  • 17
    I work a lot with IQueryable and turning those kind of warnings off is very dangerous.
    – gdoron
    Nov 23, 2011 at 11:01
  • 5
    Evaluating twice is a bad thing in my books. If the method takes IEnumerable, I may be tempted to pass a LINQ to SQL query to it, which results in multiple DB calls. Since the method signature doesn't indicate that it may enumerate twice, it should either change the signature (accept IList/ICollection) or only iterate once Nov 23, 2011 at 11:01
  • 5
    optimizing early and ruining readability and thereby the possibility to do optimizations that make a diffrence later is way worse in my book.
    – vidstige
    Nov 23, 2011 at 12:26
  • When you've got a database query, making sure it's only evaluated once already makes a difference. It's not just some theoretical future benefit, it's a measurable real benefit right now. Not only that, evaluating only once is not merely beneficial for performance, it's also needed for correctness. Executing a database query twice may produce different results, as someone may have issued an update command between the two query evaluations.
    – user743382
    Mar 31, 2015 at 8:26
  • 1
    @hvd I did not recommend no such thing. Of course you should not enumerate IEnumerables that gives different results each time you iterate it or that take a really long time to iterate. See Marc Gravells answer - He also state first and foremost "perhaps don't worry". The thing is - Lots of the times you see this you've got nothing to worry about.
    – vidstige
    Mar 31, 2015 at 9:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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