0

I have the following code:

class Action {

    public void step(Game game) {
        //if some condition met, 
        // then remove self from action stack
        game.actionStack.remove(this);

}


class Game (

    public ArrayList<Action> actionStack;

    public Game() {
        actionStack = new Arraylist<Action>();
        actionStack.add(new Action());

        while (true) {
            for (Action action : this.actionStack) {
                action.step(this);
            }
        }
    }
}

An exception gets thrown when game.actionStack.remove(this); occurs. Is there a way to remove the element safely from inside the Action class like I want?

Chandrayya G K
  • 8,719
  • 5
  • 40
  • 68
SheerSt
  • 3,229
  • 7
  • 25
  • 34
  • concurrentModificationException – Rahul Jan 15 '14 at 07:04
  • 1
    If you try to remove element from array, you may get `ConcurrentModificationException `. You need to use `Iterator` remove for this – Hüseyin BABAL Jan 15 '14 at 07:05
  • possible duplicate of [ConcurrentModificationException for ArrayList](http://stackoverflow.com/questions/3184883/concurrentmodificationexception-for-arraylist) – Dennis Meng Jan 15 '14 at 07:08

4 Answers4

4

I'm guessing you're getting a ConcurrentModificationException because you're calling the list remove method while iterating it. You can't do that.

An easy fix is to work on a copy of the array when iterating:

for (Action action : new ArrayList<>(this.actionStack)) {
    action.step(this);
}

A slightly more efficient fix is to use an explicit Iterator and call its remove method. Perhaps have step() return a boolean indicating whether it wants to remain in the list for the next step or not:

for (Iterator<Action> it = this.actionStack.iterator(); it.hasNext();) {
    Action action = it.next();
    if (!action.step(this)) {
        it.remove();
    }
}
Boann
  • 48,794
  • 16
  • 117
  • 146
1

From : the java tutorial we get the following:

Iterators

...

Note that Iterator.remove is the only safe way to modify a collection during iteration; the behavior is unspecified if the underlying collection is modified in any other way while the iteration is in progress.

Use Iterator instead of the for-each construct when you need to:

  • Remove the current element. The for-each construct hides the iterator, so you cannot call remove. Therefore, the for-each construct is not usable for filtering.
  • Iterate over multiple collections in parallel.

The following method shows you how to use an Iterator to filter an arbitrary Collection — that is, traverse the collection removing specific elements.

static void filter(Collection<?> c) {
    for (Iterator<?> it = c.iterator(); it.hasNext(); )
        if (!cond(it.next()))
            it.remove();
}

This simple piece of code is polymorphic, which means that it works for any Collection regardless of implementation. This example demonstrates how easy it is to write a polymorphic algorithm using the Java Collections Framework.

ufis
  • 176
  • 1
  • 11
0

Note: I assume, you implemented equals and hashCode methods for your class You need to use iterator to remove like below;

class Game (

    public ArrayList<Action> actionStack;

    public Game() {
        actionStack = new Arraylist<Action>();
        actionStack.add(new Action());

        while (true) {
            for (Iterator<Action> it = this.actionStack.iterator(); it.hasNext(); ) {
               it.remove();
            }
        }
    }
}

Edit: step function is doing simple remove job. I move it to Game constructor

Hüseyin BABAL
  • 15,400
  • 4
  • 51
  • 73
  • This won't work because you're still concurrently modifying the list. You have to remove the element through the *same* iterator as is used in the main loop. – Boann Jan 15 '14 at 07:34
0

I suspect that you are getting a Concurrent Modification Exception. I would suggest you do it like this

class Action {

    public void step(Game game) {
        //if some condition met, 
        // then remove self from action stack
        List<Action> tmpActionList = new List<Action>();
        tmpActionList = game.actionStack
        tmpActionList.remove(this);
        game.actionStack = tmpActionList;
    }
}

Let me know if it works.

Kainda
  • 525
  • 6
  • 17
  • 1
    This won't work because tmpActionList is not a copy. It's a pointer to the same list object. – Boann Jan 15 '14 at 07:34