3

I have a vf page that creates 10 dummy Attachments so it can display on the page. When I go to save the attachments I"m looping through the list and removing the bad ones. The problem is it's only looping 5 times and then gives the error in test:

List Index out of bounds: 5

I can't figure out why it's stopping, only halfway.

Method:

public PageReference saveAttachments()
{
    Integer startSize = attachments.size();

    for(Integer i = 0; i < startSize; i++)
    {
        if(
            attachments[i].Body == null ||
            attachments[i].Name == null
        )
        {
            attachments.remove(i);
        }
    }

    if(attachments.size() > 0)
    {
        try {
            insert attachments;
            return goBack();
        } catch (DMLException ex)
        {
            ApexPages.addMessages(ex);
        }
    }
    return null;  
} 
8

You are removing items while iterating over them.

When you get to 5 you have removed 5 items and this the size is only 5. When you get to six it breaks.

It is not advisable in general to remove items while iterating over them but if you do there are several ways to do it.

This is one, use a collection to insert and not remove items from the initial collection:

public PageReference saveAttachments()
{
    Integer startSize = attachments.size();
    Attachment[] toInsert = new Attachment[]{};

    for(Integer i = 0; i < startSize; i++)
    {
        if(
            attachments[i].Body != null &&
            attachments[i].Name != null
        )
        {
            toInsert.add(attachments[i];
        }
    }

    try {
        insert toInsert;
        return goBack();
    } catch (DMLException ex){
        ApexPages.addMessages(ex);
    }
    return null;  
} 

you can also step backwards if you want to remove items from the collection:

public PageReference saveAttachments()
{
    Integer startSize = attachments.size();

    for(Integer i = startSize -1; i >=0; i--)
    {
        if(
            attachments[i].Body == null ||
            attachments[i].Name == null
        )
        {
            attachments.remove(i);
        }
    }

    if(attachments != null) //no need to check size, depending on code can remove this if the collection is never null. Since you did not check when you get the size above you can most likely remove this if
    {
        try {
            insert attachments;
            return goBack();
        } catch (DMLException ex)
        {
            ApexPages.addMessages(ex);
        }
    }
    return null;  
} 

Among various other ways some of which are better patterns which I am sure someone will post as well

4
  • Ohhhh, I understand this. Feb 18 '20 at 21:06
  • Really good answer, I prefer the backwards counting because it doesn't use the extra array.
    – Jungkook
    Feb 19 '20 at 6:50
  • 2
    For your backwards loop, you should use for (Integer i = startSize -1;
    – HeartWare
    Feb 19 '20 at 8:03
  • @HeartWare you are correct sir. Updated
    – Eric
    Feb 19 '20 at 8:04
3

You are removing elements from the list, so the list gets smaller. By the time you get to i=5, there are only 5 elements left in the list so the final index is 4.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.