0

ReSharper says "Possible multiple enumeration of IEnumerable" on this code:

public static IEnumerable<T> Each<T>(this IEnumerable<T> @this, Action<T> action)
{
    foreach (var i in @this)
        action(i);

    return @this;
}

But I just return @this, I don't do anything else with it... is it warning me of possibility for additional enumeration once the function returns, or I'm missing something here ?

Tar
  • 8,529
  • 9
  • 56
  • 127
  • this was answered before, try this: http://stackoverflow.com/questions/8240844/handling-warning-for-possible-multiple-enumeration-of-ienumerable – Wasafa1 Apr 21 '13 at 10:21
  • 1
    Yes, it's because it knows you have enumerated it once, and the caller could enumerate it again. – Matthew Watson Apr 21 '13 at 10:57
  • @Wasafa1 - already saw that thread, TL/DR. My question is short and accurate. – Tar Apr 21 '13 at 11:23
  • It really depends on what you expect to happen. Consider the code SomeSource.Each(MyAction).FirstOrDefault(). What do you expect to happen? In your case the Each method performs MyAction on the entire list even though only the first item was requested. Also, if you dispose of the enumerator before it is emptied (eg use "break;" in a foreach loop), the entire set has been passed as parameter to an invocation of MyAction. – Tormod Apr 21 '13 at 16:18

2 Answers2

3

But I just return @this, I don't do anything else with it...

Yes, but the caller will probably also enumerate it...

Anyway, using an iterator block like you did in your answer is better, because:

  • it avoids multiple enumeration
  • it maintains deferred execution (i.e. the source sequence won't be enumerated until you start enumerating the result of Each)
Thomas Levesque
  • 286,951
  • 70
  • 623
  • 758
  • What do you mean by "it maintains deferred execution"? At first call, this enumerates the entire list and then return the enumerable. So even if the caller just used SomeSource.Each(MyAction).FirstOrDefault(), MyAction will be performed on the entire list. – Tormod Apr 21 '13 at 16:13
  • @Tormod, if you use an iterator block (yield return), then no, it doesn't enumerate the entire list. Actually, it doesn't do anything until you start enumerating the result, and then items from the source are only consumed one by one as the result is consumed. So in the example you give, MyAction would only be performed on the first item of the list. – Thomas Levesque Apr 21 '13 at 21:40
  • Ok. I didn't realize that you were not referring to the code in OP. I thought you were using the term "iterator block" losely. It would increase the readability if you post comments on other answers as comments on the answers. – Tormod Apr 22 '13 at 07:08
1

This avoids the warning and I assume that it's more efficient:

public static IEnumerable<T> Each<T>(this IEnumerable<T> @this, Action<T> action)
{
    foreach (var i in @this)
    {
        action(i);
        yield return i;
    }
}

Could someone verify that it is indeed more efficient (doesn't enumerate twice ?) ?

Tar
  • 8,529
  • 9
  • 56
  • 127
  • 1
    It is more efficient provided that it still does what he intends to do. Your code will invoke the action only on elements that are being consumed by the code calling Each(). Code in OP guaranteed that the action was invoked for the entire set as the first item was requested. Also, even if the caller of your code consumes the entire list, OP code still has a different perfomance profile than yours. His cost is "up front" whilst your code will tax the CPU while consuming the result. – Tormod Apr 21 '13 at 16:26