C# Dynamically added button click is throwing argument out of bounds exception

Question

Basically I am trying to create an attachment window utilizing keeping everything in lists for easy use later. So, every time the form loads it goes through everything in the list and creates both labels and buttons for them. There is no errors until I click my button. If I click any of the X buttons, I get an argument out of bounds exception on the click += line. What's interesting is why its being called? The click isn't supposed to add another event handler to itself. Its also interesting that on click the indicie is one greater than the total count so how its even executing that line is beside me considering it should never iterate higher that its max count. Any help would be greatly appreciated.

    ArrayList attachmentFiles;
    ArrayList attachmentNames;
    public Attachments(ArrayList attachments, ArrayList attachmentFileNames)
    {
        InitializeComponent();
        attachmentFiles = attachments;
        attachmentNames = attachmentFileNames;
    }

    private void Attachments_Load(object sender, EventArgs e)
    {
        ScrollBar vScrollBar1 = new VScrollBar();
        ScrollBar hScrollBar1 = new HScrollBar();
        vScrollBar1.Dock = DockStyle.Right;
        hScrollBar1.Dock = DockStyle.Bottom;
        vScrollBar1.Scroll += (sender2, e2) => { pnl_Attachments.VerticalScroll.Value = vScrollBar1.Value; };
        hScrollBar1.Scroll += (sender3, e4) => { pnl_Attachments.HorizontalScroll.Value = hScrollBar1.Value; };
        pnl_Attachments.Controls.Add(hScrollBar1);
        pnl_Attachments.Controls.Add(vScrollBar1);
        Label fileName;
        for (int i = 0; i < attachmentNames.Count; i++)
        {
            fileName = new Label();
            fileName.AutoSize = true;
            fileName.Text = attachmentNames[i].ToString();
            fileName.Top = (i + 1) * 22;
            pnl_Attachments.Controls.Add(fileName);
            Button btn_RemoveAttachment = new Button();
            btn_RemoveAttachment.Text = "X";
            btn_RemoveAttachment.Tag = i;
            btn_RemoveAttachment.Click += new System.EventHandler((s, e3) => removeAttachment(s, e3, attachmentFiles[i].ToString(), attachmentNames[i].ToString()));
            btn_RemoveAttachment.Top = (i + 1) * 22;
            btn_RemoveAttachment.Left = fileName.Right + 22;
            pnl_Attachments.Controls.Add(btn_RemoveAttachment);
        }
    }

    private void removeAttachment(object sender, EventArgs e, string file, string fileName)
    {
        attachmentNames.Remove(fileName);
        attachmentFiles.Remove(file);
        pnl_Attachments.Controls.Clear();
        this.Close();
    }

In my test, attachmentFiles had a count of 3 and attachmentNames had a count of 3. On form load, there are no issues. But, on button click I get an exception because somehow its trying to add another click listener to a button with i = 3 (a.k.a a 4th element)

Answer 1

The problem is not with the event subscription, but with the event handler execution.

You are running into this problem because a closure is created for the event handler, but the value i is modified by the for loop. The last value of i will be 1 + attachmentNames.Count and this will be the value used by each invocation of the event handler.

For more detail as to why this happens you can check out the question and answer here: Access to Modified Closure.

To resolve the problem, you can assign i to another variable:

var currentAttachmentIndex = i;
btn_RemoveAttachment.Click += new System.EventHandler((s, e3) => { 
    removeAttachment(s, 
                     e3,
                     attachmentFiles[currentAttachmentIndex].ToString(),
                     attachmentNames[currentAttachmentIndex].ToString())
});

Or you can use the value you already captured in the Tag property of the btn_RemoveAttachment control.

btn_RemoveAttachment.Click += new System.EventHandler((s, e3) => {
    var senderButton = (Button)s;
    var currentAttachmentIndex = (int)senderButton.Tag;
    removeAttachment(s, 
                     e3,
                     attachmentFiles[currentAttachmentIndex].ToString(), 
                     attachmentNames[currentAttachmentIndex].ToString())
});

Keep in mind, though, if you are removing items from the List, the indexes will not be valid. Understanding how closures work, however, should help you solve that problem if it arises (it looks like you close the form anyway after the first removal).

Answer 2

Presumably, the attachmentFiles[i] is what is causing the out of bounds exception, perhaps attachmentFiles has fewer elements than attachmentNames?

Why not set a breakpoint on that line and check what is causing the out of bounds exception?

Answer 3

I get an argument out of bounds exception on the click += line. What's interesting is why its being called? The click isn't supposed to add another event handler to itself

It looks like the exception is not thrown at the event subscription (+=) but at the lambda function declared in that same line

Its also interesting that on click the indicie is one greater than the total count so how its even executing that line is beside me considering it should never iterate higher that its max count. Any help would be greatly appreciated

The value of i is fixed when you assign the lambda to the event. The indexes at the attachmentFiles change as you remove elements, but the indexes used by the lambda to access it don't. Let's try an example.

let's assume we have an array with 4 attchements (index:attachment))

[0:att0, 1:att1, 2:att2, 3:att3]

And 4 buttons that execute this lambdas

[removeAt(0), removeAt(1), removeAt(2), removeAt(3)]

We click the second button and it correctly removes the second attachment from array, now we have:

[0:att0, 1:att2, 2:att3]

[removeAt(0), removeAt(1), removeAt(2), removeAt(3)]

Now we click the fourth button. It tries to remove the attachment with index 3 and the out of bounds exception is thrown because that index doesn't exist anymore (and even if it existed, it might not point to the right attachment!)

Answer 4

Another approach would be to modify your 'removeAttachment' method, and use that as your event handler for all buttons.

An example of this would be:

for (int i = 0; i < attachmentNames.Count; i++)
{
    var lbl_FileName = new Label
    {
        AutoSize = true,
        Name = "Label" + i,  // Give this a name so we can find it later
        Text = attachmentNames[i],
        Top = (i + 1) * 22
    };

    var btn_RemoveAttachment = new Button
    {
        Text = "X",
        Tag = i,
        Top = (i + 1) * 22,
        Left = lbl_FileName.Right + 22
    };
    btn_RemoveAttachment.Click += removeAttachment;

    pnl_Attachments.Controls.Add(lbl_FileName);
    pnl_Attachments.Controls.Add(btn_RemoveAttachment);
}

Then you can modify your removeAttachment method to be an EventHandler, and to detect the button and associated label using the sender As Button and Button.Tag property:

private void removeAttachment(object sender, EventArgs e)
{
    // Get associated Label and Button controls
    var thisButton = sender as Button;
    var index = Convert.ToInt32(thisButton.Tag);
    var thisLabel = (Label) Controls.Find("NameLabel" + index, true).First();

    // Remove the files
    int itemIndex = attachmentNames.IndexOf(thisLabel.Text);
    attachmentNames.RemoveAt(itemIndex);
    attachmentFiles.RemoveAt(itemIndex);

    // Disable controls and strikethrough the text
    thisButton.Enabled = false;
    thisLabel.Font = new Font(thisLabel.Font, FontStyle.Strikeout);
    thisLabel.Enabled = false;
}