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

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

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.

|improve this answer|||||

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.