Java: new instance of the same type as the ArrayList loop stops the loop

Question

The loop in the following code stops after the first element:

public ArrayList<Position> moveOptions(Player p, Die d)
    {
        // Init array
        ArrayList<Position> options = new ArrayList<>();
        Helper.m("Option size: " + options.size());
        // Get player's pin positions (current)
        ArrayList<Position> pos = p.getPinPositions();
        Helper.m("p.getPinPositions() size: " + pos.size());
        int i = 0;

        for (Position ps : p.getPinPositions()) {
            Helper.m("I am in the loop");
            // Get the pin in this position
            Pin pin = ps.getPin();
            // Get next field according to the die
            Helper.m("The size of pos before 'next' call:" + pos.size());
            Field next = ps.getPin().getNextFieldByDie(d);
            Helper.m("The size of pos after 'next' call:" + pos.size());
            // If the die value doesn't exceed the board size
            if(next != null)
            {
                Helper.m("next is not null");
                Helper.m("The size of pos before constructor call:" + pos.size());
                Position possible = new Position(next, ps.getPin());
                Helper.m("The size of pos after constructor call:" + pos.size());
                options.add(possible);
            }
            Helper.m("The size of 'options' is now " + options.size());
            i++;
        }

        Helper.m("I: " + i);
        Helper.m("The final size of 'options' is " + options.size());
        Helper.m("The final size of 'pos' is " + pos.size());
        return options;
    }

After some investigation I got to the conclusion that this line is the issue:

Position possible = new Position(next, ps.getPin());

The loop doesn't continue even if I put a 'continue' there or if I create a new empty instance of the Position outside the loop. Any suggestions?

The output is this:

Option size: 0
p.getPinPositions() size: 4
I am in the loop
The size of pos before 'next' call:4
The size of pos after 'next' call:1
next is not null
The size of pos before constructor call:1
The size of pos after constructor call:1
The size of 'options' is now 1
I: 1
The final size of 'options' is 1
The final size of 'pos' is 1

The Position class:

/**
 * Keep pin positions in object
 */
public class Position {
    // Field object
    private Field field;
    // Pin Object
    private Pin pin;

    public Position(){};
    /**
     * Constructor
     * @param f Field object
     * @param p Pin Object
     */
    public Position(Field f, Pin p)
    {
        this.setFeild(f);
        this.setPin(p);
    }

    /**
     * Field setter
     * @param f 
     */
    public final void setFeild(Field f)
    {
        this.field = f;
    }
    /**
     * Field getter
     * @return 
     */
    public Field getField()
    {
        return this.field;
    }
    /**
     * Pin setter
     * @param p 
     */
    public final void setPin(Pin p)
    {
        this.pin = p;
    }
    /**
     * Pin getter
     * @return 
     */
    public Pin getPin()
    {
        return this.pin;
    }
}

Thank you!

Answer 1

This problem is caused in your Pin class in your getCurrentField() method which is called when you call getNextFieldByDie(Die die) from your loop. The problem is that you are removing items from the iterator that are not equivalent to the current pin. This action doesn't just produce an ArrayList with the elements you are interested in, it actually modifies the backing store which in your code is in the GameManager class.

public Field getCurrentField()
{
    ArrayList<Position> positions = this.getManager().getPinPositions();

    for (Iterator<Position> it=positions.iterator(); it.hasNext();) {
        if (!it.next().getPin().equals(this))
            it.remove();
    }

    if(positions.size() > 0)
        return positions.get(0).getField();
    else
        return null;
}

After calling it.remove() on the Position objects that meet your criteria, your pos list (which is drawn from the same instance of the GameManager class) will no longer have anything but the original item left in it. Hence the change of size.

Rather than removing items from the iterator, it would be better to create a new ArrayList that contained the elements of interest.

public Field getCurrentField()
{
    ArrayList<Position> positions = this.getManager().getPinPositions();
    //Just create a new list
    ArrayList<Position> matchedPositions = new ArrayList<Position>();
    //Use the for-each syntax to iterate over the iterator
    for (Position position : positions) {
        //switch the logic and add to the new list
        if (position.getPin().equals(this))
            matchedPositions.add(position);
    }

    if(matchedPositions.size() > 0)
        return matchedPositions.get(0).getField();
    else
        return null;
}

I also want to point out that you have this same pattern occurring in multiple places in the code base you posted, and all of them could cause similar problems and should likely be switched.

It also just happens that you were using the only method for modifying a list while iterating over that list that will not throw the ConcurrentModificationException making it harder to detect that a change had been made in your original list.