views:

1901

answers:

5

So, if I try to remove elements from a Java HashSet while iterating, I get a ConcurrentModificationException. What is the best way to remove a subset of the elements from a HashSet as in the following example?

Set<Integer> set = new HashSet<Integer>();

for(int i = 0; i < 10; i++)
    set.add(i);

// Throws ConcurrentModificationException
for(Integer element : set)
    if(element % 2 == 0)
     set.remove(element);

Here is a solution, but I don't think it's very elegant:

Set<Integer> set = new HashSet<Integer>();
Collection<Integer> removeCandidates = new LinkedList<Integer>();

for(int i = 0; i < 10; i++)
    set.add(i);

for(Integer element : set)
    if(element % 2 == 0)
     removeCandidates.add(element);

set.removeAll(removeCandidates);

Thanks!

+19  A: 

You can manually iterate over the elements of the set:

Iterator<Integer> iterator = set.iterator();
while (iterator.hasNext()) {
    Integer element = iterator.next();
    if (element % 2 == 0) {
        iterator.remove();
    }
}

You will often see this pattern using a for loop rather than a while loop:

for (Iterator<Integer> i = set.iterator(); i.hasNext();) {
    Integer element = i.next();
    if (element % 2 == 0) {
        i.remove();
    }
}

As people have pointed out, using a for loop is preferred because it keeps the iterator variable (i in this case) confined to a smaller scope.

Adam Paynter
I prefer `for` to `while`, but each to his/her own.
Tom Hawtin - tackline
I also use `for` myself. I used `while` to hopefully make the example clearer.
Adam Paynter
I perfer `for` mostly because the iterator variable is then limited to the scope of the loop.
Kathy Van Stone
If `while` is used then the iterator's scope is larger than it needs to be.
Steve Kuo
I prefer the while because it looks cleaner to me. The scope of the iterator should not be an issue if you are factoring your code. See Becks book "Test Driven Development" or Fowler's "Refactoring" for more about factoring code.
Nash0
+2  A: 

you can also refactor your solution removing the first loop:

Set<Integer> set = new HashSet<Integer>();
Collection<Integer> removeCandidates = new LinkedList<Integer>(set);

for(Integer element : set)
   if(element % 2 == 0)
       removeCandidates.add(element);

set.removeAll(removeCandidates);
dfa
+1  A: 

Does it need to be whilst iterating? If all you're doing is filtering or selecting I would suggest using Apache Commons CollectionUtils. There are some powerful tools there and it makes your code "cooler."

Here's an implementation that should provide what you need:

Set<Integer> myIntegerSet = new HashSet<Integer>();
// Integers loaded here
CollectionUtils.filter( myIntegerSet, new Predicate() {
                              public boolean evaluate(Object input) {
                                  return (((Integer) input) % 2 == 0);
                              }});

If you find yourself using the same kind of predicate frequently you can pull that out into a static variable for reuse... name it something like EVEN_NUMBER_PREDICATE. Some may see that code and declare it "hard to read" but it looks cleaner when you pull out the Predicate into a static. Then it's easy to see that we're doing a CollectionUtils.filter(...) and that seems more readable (to me) than a bunch of loops all over creation.

dustmachine
+1  A: 

The reason you get a ConcurrentModificationException is because an entry is removed via Set.remove() as opposed to Iterator.remove(). If an entry is removed via remove() while an iteration is being done, you will get a ConcurrentModificationException. On the other hand, removal of entries via Iterator.remove() while iteration is supported in this case.

The new for loop is nice, but unfortunately it does not work in this case, because you can't use the Iterator reference.

If you need to remove an entry while iteration, you need to use the long form that uses the Iterator directly.

for (Iterator<Integer> it = set.iterator(); it.hasNext(); ) {
    if (element % 2 == 0) {
        it.remove(element);
    }
}
sjlee
A: 

An other possible solution:

for(Object it : set.toArray()) { /* Create a copy */
    Integer element = (Integer)it;
    if(element % 2 == 0)
        set.remove(element);
}

Or:

Integer[] copy = new Integer[set.size()];
set.toArray(copy);

for(Integer element : copy) {
    if(element % 2 == 0)
        set.remove(element);
}
alex2k8