How to correctly animate images in a data structure and not get ConcurrentModificationException

Question

For those who hate reading long questions, take the complete code below, run it, hit SPACE a few times, and you'll get a ConcurrentModificationException. Simple question: How do you fix it? The problem is trying to remove a Fireball from the list when it exits the screen. The Timer code is where the problem lies.

If you want more info, keep reading.

In this question where the OP asks how to to shoot fireball images, I answered with this answer indicating that a data structure should be used to hold the fireballs. IMO it was a half @$$ answer. The reason I think this is because the code I gave isn't complete, because it doesn't take into account when a fireball needs to be removed from the data structure, say when the fireball moves off the screen or if it were to collide with an opposing player. So ultimately it just becomes an endless List of fireballs, which I don't think is efficient nor the correct way of doing it.

Here's how I did it. There's a Fireball class which holds an image for the fireball and x and y locations. All I did was keep adding a Fireball instance to the List with a key bind and animated with timer moving the x location of the Fireball

Timer timer = new Timer(40, new ActionListener() {
    @Override
    public void actionPerformed(ActionEvent e) {
        for (Fireball ball : fireBalls) {
            ball.x += X_INC;
            repaint();
        }
    }
});
...
getActionMap().put("hadouken", new AbstractAction() {
    @Override
    public void actionPerformed(ActionEvent e) {
        fireBalls.add(new Fireball(fireball));
    }
});

---

So I said this is an incomplete answer for this reason - "because it doesn't take into account when a fireball needs to be removed from the data structure, say when the fireball moves off the screen or if it were to hit collide with an opposing player"

I did attempt to take this into account by doing this, removing the Fireball from the list if its x position exceeded the screen width

Timer timer = new Timer(40, new ActionListener() {
    @Override
    public void actionPerformed(ActionEvent e) {
        for (Fireball ball : fireBalls) {
            if (ball.x > D_W) {
                fireBalls.remove(ball);
            } else {
                ball.x += X_INC;
                repaint();
            }
        }
    }
});

The problem with this though is that once the Fireball reaches the end of the screen and is to be removed from the List, I get a ConcurrentModificationException. I searched how to fix this, and some suggested using an Iterator, but when I tried this, I still get the exception when many Fireballs exist in the List

public void actionPerformed(ActionEvent e) {
    Iterator<Fireball> it = fireBalls.iterator();
    while (it.hasNext()) {
        Fireball ball = it.next();
        if (ball.x > D_W) {
            fireBalls.remove(ball);
        } else {
            ball.x += X_INC;
            repaint();
        }
    }
}

---

So my question is, what is the correct way to animate this scenario (removing the ball from the List when it exits the screen), to avoid the ConcurrentModificationException? The Timer code is where the problem lies.

Here's the code you can run

import java.awt.*;
import java.awt.event.*;
import java.awt.image.BufferedImage;
import java.io.IOException;
import java.net.URL;
import java.util.*;
import java.util.List;
import java.util.logging.*;
import javax.imageio.ImageIO;
import javax.swing.*;
import javax.swing.Timer;

public class WannaBeStreetFighter extends JPanel {

    private static final int D_W = 700;
    private static final int D_H = 250;
    private static final int X_INC = 10;

    List<Fireball> fireBalls;
    BufferedImage ryu;
    BufferedImage fireball;
    BufferedImage background;

    public WannaBeStreetFighter() {

        try {
            ryu = ImageIO.read(new URL("http://www.sirlin.net/storage/street_fighter/ryu_hadoken_pose.png?__SQUARESPACE_CACHEVERSION=1226531909576"));
            background = ImageIO.read(new URL("http://fightingstreet.com/folders/variousinfofolder/ehondasbath/hondasfz3stage.gif"));
            fireball = ImageIO.read(new URL("http://farm6.staticflickr.com/5480/12297371495_ec19ded155_o.png"));
        } catch (IOException ex) {
            Logger.getLogger(WannaBeStreetFighter.class.getName()).log(Level.SEVERE, null, ex);
        }

        fireBalls = new LinkedList<>();

        Timer timer = new Timer(40, new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                for (Fireball ball : fireBalls) {
                    if (ball.x > D_W) {
                        fireBalls.remove(ball);
                    } else {
                        ball.x += X_INC;
                        repaint();
                    }
                }
            }
        });
        timer.start();

        InputMap inputMap = getInputMap(JComponent.WHEN_IN_FOCUSED_WINDOW);
        inputMap.put(KeyStroke.getKeyStroke("SPACE"), "hadouken");
        getActionMap().put("hadouken", new AbstractAction() {
            @Override
            public void actionPerformed(ActionEvent e) {
                fireBalls.add(new Fireball(fireball));
            }
        });

    }

    @Override
    protected void paintComponent(Graphics g) {
        super.paintComponent(g);
        g.drawImage(background, 0, 0, D_W, D_H, this);
        g.drawImage(ryu, 50, 125, 150, 115, this);
        for (Fireball ball : fireBalls) {
            ball.drawFireball(g);
        }
    }

    @Override
    public Dimension getPreferredSize() {
        return new Dimension(D_W, D_H);
    }

    private class Fireball {

        Image fireball;
        int x = 150;
        int y = 125;

        public Fireball(Image image) {
            fireball = image;
        }

        public void drawFireball(Graphics g) {
            g.drawImage(fireball, x, y, 50, 50, null);
        }
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                JFrame frame = new JFrame("Best Street Fighter ever");
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.add(new WannaBeStreetFighter());
                frame.pack();
                frame.setLocationRelativeTo(null);
                frame.setVisible(true);
            }
        });
    }
}

Answer 1

You can't remove an item from a List from within a for-each loop. I don't know the details, but I know it generally doesn't work.

Instead, get a Iterator of the List and use it's remove method instead...

Iterator<Fireball> it = fireBalls.iterator();
while (it.hasNext()) {
    Fireball ball = it.next();
    if (ball.x > D_W) {
        // You can't call this.  The Iterator is backed by the ArrayList
        //fireBalls.remove(ball);
        it.remove();
    } else {
        ball.x += X_INC;
        repaint();
    }
}

Happy fire ball spamming!