java, programming

Removing Collection items using an iterator

So, I was getting a rather mysterious Java coding bug this week, which turned out to be the result of some Virtual Machine optimisation I hadn’t previously been aware of.

The code looked a bit like this:

public void deleter(GroupSet groups){
    GroupSet removalQueue = new GroupsSet();

    for(Iterator groupsIt = groups.iterator(); 
                          groupsIt.hasNext(); ){
        Group g = (Group)
        ItemSet itemsNeedingRemoval = g.doCheck();

    for(Iterator removalIt = removalQueue.iterator(); 
                              removalIt.hasNext(); ){
        Group g = (Group);
        schrodingerRemove(g); //Shrodinger remove might 
                   //or might not remove the group from 
                   //the original group set

This seems like a quite simple it of code. We take the original set, we iterate through it to find the objects within that we might need to remove, and queue them. Then, afterwards we iterate through the queue, and call the removal method that might do the removal. (We couldn’t have called iterator.remove() in the original method because we’re not sure whether the removal will occur or not, and anyway it’s not good practice.)

So, this code looks clean and straight-forward, but guess what happens when you run it?

Error: ConcurrentModificationException

Not so good. It took a while to find out why this happened, turns out the offending line is


The addAll method, in Java 1.4’s implementation of HashSet at least, rather than adding the items one-at-a-time to your new collection (like you might imagine), in fact simply adds a pointer that causes the collection you’re adding to become part of the original set. So if you remove an item from the set your added, you also remove it from the set you added too. The reverse also applies. If you modify it iterate around all the objects and call add each time, the exception goes away.

In my case, I figured I didn’t really need the set of items anyway, that 1 was enough, and changed doCheck() to return just 1 item instead of a set. Solution by design is better than solution by force 😉


Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s