ForEach extension method not being called with Dictionary

Question

I've got a Dictionary that contains a ValueTuple:

    private static readonly Dictionary<RELAY, (byte RelayNumber, bool IsClosed)> s_relays = new Dictionary<RELAY, (byte RelayNumber, bool IsClosed)>
    {
        { RELAY.OUTPUT1, (4, false) },
        { RELAY.OUTPUT2, (9, false) },
        { RELAY.OUTPUT3, (11, false) },
    };

Later in my code I set IsClosed to true for one or more relays. I wrote a ForEach extension method for the Dictionary:

    public static IEnumerable<T> ForEach<T>(this IEnumerable<T> enumeration, Action<T> action)
    {
        Debug.Assert(enumeration != null, $"Argument {nameof(enumeration)} cannot be null!");
        Debug.Assert(action != null, $"Argument {nameof(action)} cannot be null!");

        foreach (T item in enumeration)
        {
            action(item);
            yield return item;
        }
    }

I want to call a method for each relay that is closed. For example, writing to Console.WriteLine:

Relays.Where(x => x.Value.IsClosed).ForEach(x => Console.WriteLine(x.Key));

But this does not work. However, if I include ToList() the code below does work:

Relays.Where(x => x.Value.IsClosed).ToList().ForEach(x => Console.WriteLine(x.Key));

What am I missing?

(I do realize that the ForEach extension method being called is different between these two examples with the first one [mine] never being called.)

have you try `Relays.Where(x => x.Value.IsClosed).ForEach(x => Console.WriteLine(x.Key)).ToList()`? I think your extension method not called, because you do not enumerate your enumeration — vasily.sib, Jun 14 '18 at 02:57
`However, if I include ToList() the code below does work:` It works because when you do that you aren't calling your method. You are calling a completely different method - https://msdn.microsoft.com/en-us/library/bwabdf9z(v=vs.110).aspx . — mjwills, Jun 14 '18 at 04:04

David L · Accepted Answer · 2018-06-14T03:32:21.760

3

In this case, unlike the List<T>.ForEach() implementation that iterates a collection, by using the yield return pattern, your .ForEach implementation is actually a deferred execution "filter" that will not be executed until the query is fully resolved.

In that sense it is extremely similar to the .Where() method except that instead of reducing the query set, it is performing a side-effect operation on it as the query is resolved. Neither will actually be executed until the enumeration is enumerated.

Adding .ToList() to the end of your query will execute your method as expected and resolve the query.

Alternatively, if you simply want a convenient way to iterate and perform an action on a collection, you could remove the yield return and simply iterate the collection and return it at the end:

public static IEnumerable<T> ForEach<T>(this IEnumerable<T> enumeration, Action<T> action)
{
    Debug.Assert(enumeration != null, $"Argument {nameof(enumeration)} cannot be null!");
    Debug.Assert(action != null, $"Argument {nameof(action)} cannot be null!");

    foreach (T item in enumeration)
    {
        action(item);
    }

    return enumeration;
}

However, the risk there is that you could potentially enumerate twice.

It is also worth noting that your second example

Relays.Where(x => x.Value.IsClosed).ToList().ForEach(x => Console.WriteLine(x.Key));

does not actually invoke your extension method. Rather, it invokes the List<T>.ForEach() method.

edited Jun 14 '18 at 03:32

answered Jun 14 '18 at 03:01

David L

32,885
8
62
93

In that case, is there any advantage to writing my own extension method since I still need to call `ToList()`? – Mike Lowery Jun 14 '18 at 03:07
You can iterate your enumeration in this extension method `var collection = enumeration.ToList(); foreach (T item in collection) action(item); return collection;` – vasily.sib Jun 14 '18 at 03:10
@DiskCrasher I believe you don't need to return a IEnumerable from you ForEach() extension method. I don't have a computer with Visual Studio to try this out atm but I think making the method return something else (maybe an int counter showing how many object got iterated) could make it work as you intend without the ToList(). – Janilson Jun 14 '18 at 03:15
@DiskCrasher I've updated my answer to help add some additional clarity. – David L Jun 14 '18 at 03:18
@Lucas while that is true if the implementation is an extension method, that isn't true if the method implements `yield return`, as I've mentioned in my answer. – David L Jun 14 '18 at 03:20
Removing the `yield return` fixes this issue but results in an `InvalidOperationException` stating 'Collection was modified; enumeration operation may not execute.' I may just stick to using `ToList()` since that resolves the issue. – Mike Lowery Jun 14 '18 at 20:38
@DiskCrasher I can't reproduce that issue. For me the query executes correctly. Do you have an expanded snippet that causes that problem? – David L Jun 14 '18 at 20:50
@DavidL I do not. I believe it's the same issue reported [here](https://stackoverflow.com/questions/604831/collection-was-modified-enumeration-operation-may-not-execute). – Mike Lowery Jun 14 '18 at 21:40
@DiskCrasher this is is most likely an example of the multiple enumerations possibility that I mentioned. In your case, `.ToList()` at the end is probably the cleanest solution. – David L Jun 14 '18 at 22:02

CodingYoshi · Answer 2 · 2018-06-14T03:38:15.963

It is just a query and it will only be executed "lazily" when needed. Let's make things simple so we can see what happens:

private static readonly List<int> nums = new List<int> { 1, 2, 3 };

public static IEnumerable<int> MyEach(this IEnumerable<int> enumeration, Action<int> action)
{ 
    foreach (var item in enumeration)
    {
        action(item);
        yield return item;
    }
}

This is just a query, so nothing will execute yet:

var query = nums.Where(x => x == 1).MyEach(x => Console.WriteLine(x));

But as soon as you do this:

query.ToList();

It will execute MyEach extension method.

You have to be careful because it depends when it gets executed. For example, try this and you will see the number 1 printed to the console 3 times:

var query = nums.Where(x => x == 1).MyEach(x => Console.WriteLine(x));
nums.Add(1);
nums.Add(1);
query.ToList();

However, if you removed the yield, then it will execute right away:

public static void MyEach(this IEnumerable<int> enumeration, Action<int> action)
{ 
    foreach (var item in enumeration)
    {
        action(item);
        //yield return item;
    }
}

// Now it will execute right away
nums.Where(x => x == 1).MyEach(x => Console.WriteLine(x));

ForEach extension method not being called with Dictionary

2 Answers2