Monotouch.Dialog StyledStringElement delegate always fires for the last element added

Question

given the following code, I am having an issue when clicking on each element. If we assume I have 5 exercises and therefore create 5 elements in the foreach() loop, when the table is rendered and I click on any element, the delegate always gets the exercise of the 5th (last) element.

The elements are displayed properly, each showing the associated exercise's name. It is just the delegate that does not work as expected.

If I do not use a foreach loop and hardcode each element instead it works as expected. However if I cannot dynamically populate the dialogViewController and use the element tapped event for each one, is not good.

private void CreateExerciseTable()
{
Section section = new Section();

foreach (var exercise in exercises)
{
    var element = new StyledStringElement(exercise.ExerciseName, 
        delegate { AddExercise(exercise); }) 
        {
            Font = Fonts.H3,
            TextColor = UIColor.White,
            BackgroundColor = RGBColors.LightBlue,
            Accessory = UITableViewCellAccessory.DisclosureIndicator
        };

    section.Elements.Add(element);
}

var root = new RootElement("Selection") {
    section
};

 var dv = new DialogViewController(root, true);
dv.Style = UITableViewStyle.Plain;

//Remove the extra blank table lines from the bottom of the table.
UIView footer = new UIView(new System.Drawing.RectangleF(0,0,0,0));
dv.TableView.TableFooterView = footer;

dv.TableView.SeparatorColor = UIColor.White;
dv.TableView.BackgroundColor = UIColor.White;
tableFitnessExercises.AddSubview(dv.View);      
}

private void AddExercise(FitnessExercise exercise)
{
NavigationManager.FitnessRoutine.Add(exercise);
PerformSegue(UIIdentifierConstants.SegAddExerciseToFitnessRoutine, this);
}

score 8 · Accepted Answer · answered Oct 29 '12 at 22:41

8

This is a classic closure bug!

The problem is that you are accessing the loop reference.

Try:

foreach (var exercise in exercises)
{
    var localRef = exercise;
    var element = new StyledStringElement(exercise.ExerciseName, 
        delegate { AddExercise(localRef); }) 
        {
            Font = Fonts.H3,
            TextColor = UIColor.White,
            BackgroundColor = RGBColors.LightBlue,
            Accessory = UITableViewCellAccessory.DisclosureIndicator
        };

    section.Elements.Add(element);
}

For more on this see http://blogs.msdn.com/b/ericlippert/archive/2009/11/12/closing-over-the-loop-variable-considered-harmful.aspx

answered Oct 29 '12 at 22:41

Stuart

66,722
7
114
165

Well I guess I never had this happen to me before. A rookie mistake even after so many years of development. Thanks for the answer. It works fine! – Apollonas Oct 30 '12 at 19:34
To be fair, I only notice it now because Resharper warns me about it... and because I write javascript code where it happens all the time. Please do note that link from Eric Lippert - the code changed in C#5 - so in C#5 your original code would work – Stuart Oct 30 '12 at 20:09
1

If I could upvote this more than once I would... Same problem. Thank you so much for your answer. You saved me hours of having to implement a different solution! Thank you many times over! – BRogers Feb 14 '13 at 20:25

Monotouch.Dialog StyledStringElement delegate always fires for the last element added

1 Answers1