Removing objects from vectors

Newbie & Debugging Questions
#1

It doesn’t work out too well.

Every time I remove an object from my vector, this happens:


java.util.ConcurrentModificationException
        at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
        at java.util.AbstractList$Itr.next(AbstractList.java:343)
        at com.javadaemon.GameEngine.EntityManager.update(EntityManager.java:42)
        at com.javadaemon.GameEngine.GameEngine.update(GameEngine.java:59)
        at org.newdawn.slick.GameContainer.updateAndRender(GameContainer.java:657)
        at org.newdawn.slick.AppGameContainer.gameLoop(AppGameContainer.java:408)
        at org.newdawn.slick.AppGameContainer.start(AppGameContainer.java:318)
        at com.javadaemon.GameEngine.GameEngine.main(GameEngine.java:75)
Sun Feb 13 16:20:07 CET 2011 ERROR:Game.update() failure - check the game code.
org.newdawn.slick.SlickException: Game.update() failure - check the game code.
        at org.newdawn.slick.GameContainer.updateAndRender(GameContainer.java:663)
        at org.newdawn.slick.AppGameContainer.gameLoop(AppGameContainer.java:408)
        at org.newdawn.slick.AppGameContainer.start(AppGameContainer.java:318)
        at com.javadaemon.GameEngine.GameEngine.main(GameEngine.java:75)
BUILD SUCCESSFUL (total time: 8 seconds)

I thought you could remove entries in vectors?

This is my code:


public Vector<Entity> entities;

    public EntityManager() {
        entities = new Vector();
    }

public void update(int delta) {
    for (Entity e : entities) {
            e.update(delta);
            if (e instanceof Rocket) {
                Rocket rocket = (Rocket) e;
                if (rocket.isExploded) {
                    entities.remove(e);
                }
            }
        }

How do I handle vectors correctly, because in the javadocs it sais that vectors can shrink and grow as needed?

EDIT: It seems that when I tweak the update amount on the rockets the problem disappears.
This is the rocket update():


@Override
    public void update(int delta) {
        if (timePassed + delta > updateTime) {
            y -= 1;
            timePassed = 0;
            if (y < World.MINIMUM_EXPLODE_HEIGHT) {
                isExploded = true;
            }
        } else {
            timePassed += delta;
        }
    }

The Y int of the rocket starts at around 800, and MINIMUM_EXPLODE_HEIGHT is around 100.

This is how entities are updated:


public void addRocket() {
        int x = generator.nextInt(GameEngine.width);
        Rocket r = new Rocket(x, 5, 20);
        entities.add(r);
    }

    public void update(int delta) {
        timePassed += delta;
        if (timePassed > addRocketTime) {
            timePassed = 0;
            addRocket();
        }
        
        for (Entity e : entities) {
            e.update(delta);
            /*if (e instanceof Rocket) {
                Rocket rocket = (Rocket) e;
                if (rocket.isExploded) {
                    entities.remove(e);
                }
            }*/
        }

        for (Entity e : entities) {
            if (e instanceof Rocket) {
                Rocket rocket = (Rocket) e;
                if (rocket.isExploded) {
                    entities.remove(e);
                }
            }
        }
    }
#2

You can’t modify the list in the for-each loop.

#3

When you iterate using a for-each there is secretly an iterator in the background. If you remove things from the vector whilst using the iterator, the iterator gets out of sync with your vector. The exception is deliberate to point this out, because it helps to ensure your interacting with it in a safe way.

A simple fix is to iterate using an index, incrementing it when you are not removing an element. Like…


public Vector<Entity> entities;
 
public EntityManager() {
    entities = new Vector();
}
 
public void update(int delta) {
    int i = 0;

    while ( i < entities.size() ) {
        Entity e = entities.get(i);
        e.update(delta);

        if (e instanceof Rocket) {
            Rocket rocket = (Rocket) e;

            if (rocket.isExploded) {
                entities.remove(i);
            } else {
                i++;
            }
        }
    }
}

Alternatively if you use an Iterator, which is the proper way to do this. Like…


public Vector<Entity> entities;
 
public EntityManager() {
    entities = new Vector();
}
 
public void update(int delta) {
    Iterator<Entity> it = entities.iterator();

    while ( it.hasNext() ) {
        Entity e = it.next();
        e.update(delta);

        if (e instanceof Rocket) {
            Rocket rocket = (Rocket) e;
            
            if (rocket.isExploded) {
                it.remove();
            }
        }
    }
}

A third option is to record which items you are removing and then remove them later. Sometimes this is desirable.


public Vector<Entity> entities;
public Vector<Entity> removedEntities;
 
public EntityManager() {
    entities = new Vector<Entity>();
    removedEntities = new Vector<Entity>();
}
 
public void update(int delta) {
    for ( Entity e : entities ) {
        e.update(delta);
        
        if (e instanceof Rocket) {
            Rocket rocket = (Rocket) e;
            
            if (rocket.isExploded) {
                removedEntities.add( e );
            }
        }
    }
    
    entities.removeAll( removedEntities );
    removedEntities.clear();
}

All code posted above is untested, but I hope that helps!

#4

Ahh… That did the trick! :smiley: Thank you very much.
Appriciate+

#5

Or you could use a CopyOnWriteArrayList

#6

That would be much more expensive then you need it to be.

#7

It depends on how often you change the list.

But I have to admit I try my best to suppress my urges to optimize. I try to write the easiest, most readable code I can, unless I’m absolutely sure there will be a performance problem.

#8

Normally I’d agree and advocate the same argument, but using an iterator (or one of the other alternatives I’ve suggested) instead of a CopyOnWriteArrayList isn’t going to make the code that much different or less redable.