Question about foreach and delegates

Question

Suppose the following code:

foreach(Item i on ItemCollection)
{
   Something s = new Something();
   s.EventX += delegate { ProcessItem(i); };
   SomethingCollection.Add(s);
}

Of course, this is wrong because all the delegates points to the same Item. The alternative is:

foreach(Item i on ItemCollection)
{
   Item tmpItem = i;
   Something s = new Something();
   s.EventX += delegate { ProcessItem(tmpItem); };
   SomethingCollection.Add(s);
}

In this case all the delegates point to their own Item.

What about this approach? There is any other better solution?

Answer 1

UPDATE: There is extensive analysis and commentary on this issue here:

http://ericlippert.com/2009/11/12/closing-over-the-loop-variable-considered-harmful-part-one/

---

This is an extremely frequently reported issue; usually it is reported as a compiler bug, but in fact the compiler is doing the right thing according to the specification. Anonymous functions close over variables, not values, and there is only a single foreach loop variable. Therefore each lambda closes over the same variable, and therefore gets the current value of that variable.

This is surprising to almost everyone, and leads to much confusion and many bug reports. We are considering changing the specification and implementation for a hypothetical future version of C# so that the loop variable is logically declared inside the looping construct, giving a "fresh" variable every time through the loop.

This would be a breaking change, but I suspect the number of people who depend on this strange behaviour is quite low. If you have opinions on this subject, feel free to add comments to the blog posts mentioned up the update above. Thanks!

Answer 2

The second chunk of code is just about the best approach you can get all other things staying the same.

However, it may be possible to create a property on Something which takes Item. In turn the event code could access this Item off the sender of the event or it might be included in the eventargs for the event. Hence eliminating the need for the closure.

Personally I've added "Elimination of Unnecessary Closure" as a worthwhile refactoring since it can be difficult to reason on them.

Answer 3

If ItemCollection is a (generic) List you can use its ForEach-method. It will give you a fresh scope per i:

ItemCollection.ForEach(
    i =>
    {
        Something s = new Something();
        s.EventX += delegate { ProcessItem(i); };
        SomethingCollection.Add(s);
    });

Or you can use any other suitable Linq-method - like Select:

var somethings = ItemCollection.Select(
        i =>
        {
            Something s = new Something();
            s.EventX += delegate { ProcessItem(i); };
            return s;
        });
foreach(Something s in somethings)
    SomethingCollection.Add(s);

Answer 4

The problem you are facing here is related to such language construct as closure. Second piece of code fixes the problem.

Answer 5

foreach(Item i on ItemCollection)
{
   Something s = new Something(i);
   s.EventX += (sender, eventArgs) => { ProcessItem(eventArgs.Item);};
   SomethingCollection.Add(s);
}

would you not just pass 'i' in into your 'Something' Class and use it in EventX's event args