Anonymous method for event handler in for loop

Question

Can this be done in a for loop?

        TickEventArgs targs1 = new TickEventArgs(lbl1_up_time, _elapsedTime_up1);
        timer_up1.Tick += (sender, e) => Tick(targs1);

        TickEventArgs targs2 = new TickEventArgs(lbl2_up_time, _elapsedTime_up2);
        timer_up2.Tick += (sender, e) => Tick(targs2);

        TickEventArgs targs3 = new TickEventArgs(lbl3_up_time, _elapsedTime_up3);
        timer_up3.Tick += (sender, e) => Tick(targs3);

        TickEventArgs targs4 = new TickEventArgs(lbl4_up_time, _elapsedTime_up4);
        timer_up4.Tick += (sender, e) => Tick(targs4);

        TickEventArgs targs5 = new TickEventArgs(lbl5_up_time, _elapsedTime_up5);
        timer_up5.Tick += (sender, e) => Tick(targs5);

This doesnt work because i is out of bounds (5)

        targs[0] = new TickEventArgs(lbl1_up_time, _elapsedTime_up1);
        targs[1] = new TickEventArgs(lbl2_up_time, _elapsedTime_up2);
        targs[2] = new TickEventArgs(lbl3_up_time, _elapsedTime_up3);
        targs[3] = new TickEventArgs(lbl4_up_time, _elapsedTime_up4);
        targs[4] = new TickEventArgs(lbl5_up_time, _elapsedTime_up5);

        timers[0] = timer_up1;
        timers[1] = timer_up2;
        timers[2] = timer_up3;
        timers[3] = timer_up4;
        timers[4] = timer_up5;

        int i = 0;

        for (i = 0; i <= 4; i++)
        {
            timers[i].Tick += (sender, e) => Tick(targs[i]);
        }

This is coming from the lambda expression; `i` is shared between all of them. By the time the function is executed they're essentially being called like `timers[i].Tick += (sender, e) => Tick(targs[5])`. Declare a local `int locali = i` and use that in your line instead. — Chris Sinclair, Aug 24 '13 at 23:19
possible duplicate of [C# Captured Variable In Loop](http://stackoverflow.com/questions/271440/c-sharp-captured-variable-in-loop) — nawfal, Nov 02 '13 at 06:25

score 10 · Accepted Answer · edited May 23 '17 at 11:53

10

This is coming from the lambda expression; i is shared between all of them. By the time the function is executed they're essentially being called like timers[i].Tick += (sender, e) => Tick(targs[5]).

To avoid this, create a locally scoped variable (int locali = i) and use that in your line instead. This will make sure that each lambda expression actually gets the value you expect.

for (i = 0; i <= 4; i++)
{
    int locali = i;
    timers[locali].Tick += (sender, e) => Tick(targs[locali]);
}

i becomes 5 from the last iteration of your loop before exiting. Naturally, you don't have a targs[5] element, so it throws an IndexOutOfRangeException.

Technically, you don't need to use locali for the timers[i].Tick part since it's evaluated immediately, but I personally find it confusing to mix the two.

Some additional reading on the concepet:

The foreach identifier and closures

Closing over the loop variable considered harmful

edited May 23 '17 at 11:53

Community

1
1

answered Aug 24 '13 at 23:21

Chris Sinclair

22,858
3
52
93

Thanks for the quick answer. Works as it should now. Btw, do you know if there is a way to simplify the assignment of arrays, i.e all targs would be done in one statement? – dirtyw0lf Aug 25 '13 at 00:05
@dirtyw0lf: But you have them all assigned to different timers. Did you really want just a _single_ timer to clear the entire `targs` array? – Chris Sinclair Aug 25 '13 at 02:05
No, just wondering if I could do it in one pass for all timers for array assignment – dirtyw0lf Aug 26 '13 at 13:48
@dirtyw0lf: You mean skip the `for` loop and have a single line like this: `timers[magic].Tick += (sender, e) => Tick(targs[magic])`? I don't think so. You could create a separate object to handle all the `Timer`/`TickEventArgs` instances for you so you have a clean API that you then just have a single event that you can assign an `Action` delegate to. – Chris Sinclair Aug 26 '13 at 13:52
No, I mean this: targs[X] = new TickEventArgs(lblX_up_time, _elapsedTime_upX) in a loop where X would be an interation over all the controls found, before using the for loop. – dirtyw0lf Aug 26 '13 at 16:52
1

@dirtyw0lf: Ahh. Possibly with reflection, but it'd be messy. You could also store a collection of the controls so you can just look them up like `up_time_labels[X]` and `elapsedTimeUps[X]`, but you'd still have to populate those collections in a similar manner as you're doing now. What you got isn't _too bad_ anyway; hard to really suggest more improvements without a better understanding of the full scope and usage of your code. Maybe you can post something in http://codereview.stackexchange.com/ – Chris Sinclair Aug 26 '13 at 17:21

score 8 · Answer 2 · answered Aug 24 '13 at 23:21

There is only one i in this case and all of the lambdas are capturing the same value. Use a local that is scoped to the loop so that each lambda has a different copy

for (i = 0; i <= 4; i++)
{
  int j = i;
  timers[j].Tick += (sender, e) => Tick(targs[j]);
}

Anonymous method for event handler in for loop

2 Answers2