java.lang.IllegalStateException in iterator.remove()

Rocket class contains: canCarry(Item item)>checks if this item can be carried/ carry updates the weight with total weight.

U2 class is child of Rocket contains: currentweight, maxWeight=18 tons Item class contains: name to be shipped & weight.

In the method loadU2 I am trying to access a list of items and adding it into one rocket until maxWeight of that rocket is reached . For example I have 216 tons of items to carry returning a list of 12 ships.

It throws me java.lang.IllegalStateException error in the line iterator.remove(). I do not know how to go about it, but it looks like it is not allowing me to remove the items while iterating.

public ArrayList<Rocket> loadU2(ArrayList<Item> loadItems){
    //list of ships
    ArrayList<Rocket> U2Ships = new ArrayList<Rocket>();
    for(Iterator<Item> iterator = loadItems.iterator(); iterator.hasNext();) {      
        //create a new ship
        Rocket tempShip = new U2();
        Item tempItem = iterator.next();
        //loop over items check if it can be filled then remove the item that was filled.
        while(tempShip.currentWeight<tempShip.weightLimit) {
            if(tempShip.canCarry(tempItem)){
                tempShip.carry(tempItem);
                iterator.remove();
            }           
        }
        U2Ships.add(tempShip);
    }
    return U2Ships;
}   


Exception in thread "main" java.lang.IllegalStateException
    at java.base/java.util.ArrayList$Itr.remove(ArrayList.java:980)
    at Simulation.loadU1(Simulation.java:35)
    at Main.main(Main.java:14)

Simplified example of what the code is doing: Assuming maxWeight for each ship = 11 tons ArrayList loadItems = [3,5,5,8,1,2,3,5] tons

 - Ship[1]=[3,5,1,2]
 - new list to iterate over >> [5,8,3,5]
 - Ship[2]=[5,3]
 - new list to iterate over >> [8,5]
 - Ship[3]=[8]
 - new list to iterate over >> [5]
 - Ship[4]=[5]

Solution

Please, rewrite your code by creating new ArrayList, instead of changing the existing list inside its own iterator:

public ArrayList<Rocket> loadU2(ArrayList<Item> loadItems){
    //list of ships
    ArrayList<Rocket> U2Ships = new ArrayList<Rocket>();
    ArrayList<Item> updatedLoadItems = new ArrayList<Item>();
    for(Iterator<Item> iterator = loadItems.iterator(); iterator.hasNext();) {      
        //create a new ship
        Rocket tempShip = new U2();
        Item tempItem = iterator.next();
        //loop over items check if it can be filled then only leave the load item that was not fully filled.
        boolean addLoadItem = true;
        while(tempShip.currentWeight<tempShip.weightLimit) {
            if(tempShip.canCarry(tempItem)){
                tempShip.carry(tempItem);
                addLoadItem = false;
            }         
        }
        if (addLoadItem) {
          updatedLoadItems.add(tempItem);
        };
        U2Ships.add(tempShip);
    }
    loadItems.removeAll();
    loadItems.addAll(updatedLoadItems);
    return U2Ships;
}

This is not the best solution, but to provide a better solution, you need to change the signature of public ArrayList<Rocket> loadU2(ArrayList<Item> loadItems)

You can try to improve your code by refactoring it.

Hint: right now your loadU2 method tries to do both things at the same time: change loadItems and create U2Ships. This is a direct violation of the single responsibility principle. Try to imagine the soldier who would try to shoot the gun and throw grenade at the same time! One thing at the time.