Old for-each loop works, new for-each loop gets ConcurrentModificationException

Question

From what I've read I understand that you get a ConcurrentModificationException when you try to edit a list while it's still being iterated.

Now what I don't get is, why does the old foreach loop not give the exception while the the new foreach loop does?

public void newForeachLoop() {
    for (Person person : list) {
        if (person.getPosition().equals(this.getPosition())) {
            list.remove(person);
        }
    }
}

public void oldForeachLoop() {
    for (int i = 0; i < list.size(); i++) {
        if (list.get(i).getPosition().equals(this.getPosition())) {
            list.remove(list.get(i));
        }
    }
}

@Sky ... check this out http://stackoverflow.com/questions/9806421/concurrentmodificationexception-when-adding-inside-a-foreach-loop-in-arraylist — digidude, Aug 17 '15 at 15:26
`oldForeachLoop` doesn't use an iterator. No iterator, no problem with the list being modified. — Ian Newson, Aug 17 '15 at 15:28
@digidude I was trying to write something similar but you were the first:) Actually, I think you should make your comment an answer with some explanations — Chaosit, Aug 17 '15 at 15:29
That said, `oldForeachLoop` has a bug in that the index is being incremented even if an element is removed, so you might get some elements in the list being missed. — Ian Newson, Aug 17 '15 at 15:29
To clarify, there is only a single for-each loop in your code. the "oldForeachLoop" is just a simple for loop. — CubeJockey, Aug 17 '15 at 15:29
Newer version is simply safer. Iterator used by new for-each copies current value representing number of modifications of list, and if this number doesn't match will throw exception (value is updated if removing is done via iterator, but will not be updated if it is done by list). There is no such test in `get(i)` used in old loop. — Pshemo, Aug 17 '15 at 15:29
@Chaosit ... I actually would have but this is a duplicate question with pretty detailed answers on the other post. That's why I just directed Sky to that post. :) — digidude, Aug 17 '15 at 15:30
Actually `oldForeachLoop` is a conventional for loop and it is also not recomennded to modify the collection while iterating through it in such way. If you want to modify the collection then it's safer to use Iterator — Chaosit, Aug 17 '15 at 15:30
So if I understand this correctly using the 'oldForeachLoop' to edit the arraylist is bad practice and I should instead add the modifications to a new list and then after the loop is done, change the main loop? — SJ19, Aug 17 '15 at 15:31
BTW, since Java 8 we can use `list.removeIf(filter)` where *`filter` a predicate which returns true for elements to be removed*. — Pshemo, Aug 17 '15 at 15:34

score 1 · Answer 1 · answered Aug 17 '15 at 15:32

1

In the old loop you're not using the lists iterator instead you're using a count of the objects in the list.

In the new loop you're using the built-in iterator which is a pointer for that instance. When you remove an item from the list you're modifying that instance and resetting the iterator thus throwing the exception.

answered Aug 17 '15 at 15:32

Setherith

329
2
10

So would you consider it bad practice to modify the list in the old loop? – SJ19 Aug 17 '15 at 15:33
Personally I wouldn't modify a list I was iterating through, I would copy the result to another list. – Setherith Aug 17 '15 at 15:38
And how about cases where you don't need to keep iterating after you call remove? Is it okay to just call return; then? Or is it still better practice to just add it all to a list? – SJ19 Aug 17 '15 at 15:40
1

To make this answer complete, you can use the iterator to remove an element. See iterator interface doc. To do that, you can call the iterator method on the collection that implements iterable. You then use the iterator next method to traverse the collection with the option to call remove. – Tarik Aug 17 '15 at 15:40
why not use CopyOnWriteArrayList? http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/CopyOnWriteArrayList.html – Sudhansu Choudhary Aug 17 '15 at 15:42
You could copy the list and remove the unwanted item(s) from it and return or if you use some external counter (i.e. your old loop) to iterate through the list you could break and return. – Setherith Aug 17 '15 at 15:48

score 1 · Answer 2 · answered Aug 19 '15 at 06:53

Because for each loop is iterator based, you can't just remove an element from the list while iterating over it. You can even try explicitly using iterator and removing an element.

    List<String> list= new ArrayList <String>;
    list.add("One");
    list.add("two");
    list.add("three");

    Iterator listItr = list.iterator () ;
    while ( listItr.hasNext() )
    {
      String countStr = itr.next();
      if ( countStr.equals ("two"))
          itr.remove(); //will not throw any exception

     //if you do it list.remove (countStr) //will throw exception 
    }

Removing an element from list using index while iterating over it, will definitely not throw any exception but you need to be extra careful about its length getting modified. Even indexes of further elements are also disturbed by your operation. So if you take care of this its not a problem.

score 0 · Answer 3 · answered Aug 19 '15 at 07:05

As @SacJn explained, you cannot make structural changes in the List (e.g. add or remove elements) while iterating it via iterator(). The iterator() will detect the inconsistency and throw a ConcurrentModificationException. In Java-8 there's clean and safe way to solve your task:

public void java8Solution() {
    list.removeIf(person -> person.getPosition().equals(this.getPosition()));
}

Old for-each loop works, new for-each loop gets ConcurrentModificationException

3 Answers3