0

I have an object that is in an arraylist, called a PowerUp. I want these to be clickable, and when they are clicked, they will be removed from the screen, and ultimately taken out of the arraylist. I have inserted the handler into the class HealthPack, which in turn extends PowerUp. I am trying to access the certain HealthPack that was clicked on and remove it from that list. I keep getting either it not working correctly, or a ConcurrentModificationException. Here is my code I am trying to work with:

for (int i = 0; i < ((SurvivalMode) m).getPowerUps().size(); i++) {
    PowerUp p = ((SurvivalMode) m).getPowerUps().get(i);
    if (p.equals(hp)) { // HealthPack hp = this;
        ((SurvivalMode) m).getPowerUps().remove(p);
        addPoints();
    }
}

This current code actually throws a ConcurrentModificationException when I go to click on a HealthPack when the list is both adding it, and another is iterating through it. I have tried synchronizing the methods that mess with the list, but it didn't help.

How would I keep my program from throwing a ConcurrentModificationException if one method is trying to remove an element from the list while another one is either iterating through the list or one is adding or removing an element from the list?

EDIT:
Here is some additional code that actually modifies the arraylist for the items:

if (powerups.size() >= 15 || isPaused()) return;

    int gen = random.nextInt(10);
    if (gen == 0) {
        powerups.add(new HealthPack(this));
        addMouseListener(powerups.get(powerups.size() - 1).getMouseListener());
    }
}

and some code that actually iterates through that list (which throws the ConcurrentModificationException):

for (PowerUp p : powerups) p.update();

CURRENT METHOD: Here is the current method that I have attempted to remove from the list on click, but it still doesn't work so well, as in it doesn't remove anything at all or it will remove the wrong one, and sometimes even calls the method for all of the other PowerUps in the list:

Iterator<PowerUp> iter = ((SurvivalMode) m).getPowerUps().iterator();
while (iter.hasNext()) {
    PowerUp p = (HealthPack) iter.next();
    if (p.equals(hp)) {
        ((SurvivalMode) m).getPowerUps().remove(p);
    }
    CellDefender.getSounds().play(SoundType.HEALTH_PACK);
    break;
}

Update 2:
What I have recently done is actually copy the array list within another point, and it partially helps to reduce the errors (within my update method):

CopyOnWriteArrayList<PowerUp> cpowerups = new CopyOnWriteArrayList<PowerUp>();

for (int i = 0; i < powerups.size(); i++) {
    cpowerups.add(powerups.get(i));
}

for (PowerUp p : cpowerups) p.update();

And I would like to ask one thing, is there a way to detect if a list is currently being modified, and if the list is being modified to break out of the loop?

CoderMusgrove
  • 604
  • 8
  • 18

2 Answers2

0

You have to use Iterator for loop to remove elements from ArrayList.

Iterator<PowerUp> iter = ((SurvivalMode) m).getPowerUps().iterator();
while(iter.hasNext()) {
   PowerUp p = iter.next();
   // your conditions to remove element here
   iter.remove();
}
Alex
  • 11,451
  • 6
  • 37
  • 52
  • How would I be able to get the index from that iterator? This is the code I am attempting: `Iterator iter = ((SurvivalMode) m).getPowerUps().iterator(); while (iter.hasNext()) { PowerUp p = ((SurvivalMode) m).getPowerUps().get(???); if (iter.equals((p))) iter.remove(); }` – CoderMusgrove Nov 27 '13 at 02:16
  • You don't need index. `if (p.equals(hp)) iter.remove();` – Alex Nov 27 '13 at 04:49
  • When I do that, it doesn't actually do a thing with the code within the `if` statement; I tried taking out that if, and it threw an `IllegalStateException` at `iter.remove();` – CoderMusgrove Nov 27 '13 at 05:41
  • I have tried that, didn't work well, and the `iter.remove()` method still throws the `ConcurrentModificationException`. I tried using a check if `p.equals(hp)` and it works halfway, though. **NOTE:** I have updated the post, please take a look. – CoderMusgrove Nov 27 '13 at 18:46
0

Since I don't know your entire code, I have to make some assumptions. My first assumption is, that your problematic code fragment is called somehow by the update method of the PowerUp class.

As stated in [1], a for each loop uses an Iterator object to iterate over the elements of an ArrayList.

Those Iterator objects returned by an ArrayList are fail-fast. That is, their methods throw a ConcurrentModificationException if the ArrayList is modified in any way after the creation of such an Iterator object, except through the object itself. (cf. [2])

If my assumption is correct, your code for (PowerUp p : powerups) p.update(); creates such an Iterator object and modifies the ArrayList within the other given code fragment. That is the reason why you encounter the same exception with the code proposed by Alex.

A solution of your problem is the use of a CopyOnWriteArrayList whenever you iterate over a Collection (ArrayList, LinkedList, etc.) It creates a shallow copy of the collection and iterates over the elements of the copy, so that you can modify the original collection without the occurrence of a ConcurrentModificationException. That means, you have to replace for (PowerUp p : powerups) p.update(); with for (PowerUp p : CopyOnWriteArrayList(powerups) p.update(); and use

Iterator<PowerUp> iter = ((SurvivalMode) m).getPowerUps().iterator();
while(iter.hasNext()) {
    PowerUp p = iter.next();
    // your conditions to remove element here
   iter.remove();
}

as proposed by Alex.

Community
  • 1
  • 1
ojlr
  • 66
  • 3
  • `iter.remove()` isn't usable in the `CopyOnWriteArrayList` class, it throws an `UnsupportedOperationException`. – CoderMusgrove Nov 27 '13 at 22:33
  • Ah, sorry. I see what the problem is. I formulated it unclear. So, here my correction: Iterating over your elements, you have to use the `CopyOnWriteArrayList` **except** in those cases when you want to modify your collection (in your case, delete an element). Here you have to use the original collection as proposed by @Alex. – ojlr Nov 28 '13 at 11:10