Hashmap.keySet(), foreach, and remove

Question

I know that it's typically a big no-no to remove from a list using java's "foreach" and that one should use iterator.remove(). But is it safe to remove() if I'm looping over a HashMap's keySet()? Like this:

for(String key : map.keySet()) {
  Node n = map.get(key).optimize();
  if(n == null) {
   map.remove(key);
  } else {
   map.put(key, n);
  }
}

Answer 1

EDIT:

I hadn't noticed that you weren't really adding to the map - you were just changing the value within the entry. In this case, pstanton's (pre-edit¹) solution is nearly right, but you should call setValue on the entry returned by the iterator, rather than calling map.put. (It's possible that map.put will work, but I don't believe it's guaranteed - whereas the docs state that entry.setValue will work.)

for (Iterator<Map.Entry<String, Node>> it = map.entrySet().iterator(); 
     it.hasNext();)
{
    Map.Entry<String, Node> entry = it.next();
    Node n = entry.getValue().optimize();
    if(n == null) 
    {
        it.remove();
    }
    else
    {
        entry.setValue(n);
    }
}

(It's a shame that entry doesn't have a remove method, otherwise you could still use the enhanced for loop syntax, making it somewhat less clunky.)

Old answer

(I've left this here for the more general case where you just want to make arbitrary modifications.)

No - you should neither add to the map nor remove from it directly. The set returned by HashSet.keySet() is a view onto the keys, not a snapshot.

You can remove via the iterator, although that requires that you use the iterator explicitly instead of via an enhanced for loop.

One simple option is to create a new set from the original:

for (String key : new HashSet<String>(map.keySet())) {
    ...
}

At this point you're fine, because you're not making any changes to the set.

EDIT: Yes, you can definitely remove elements via the key set iterator. From the docs for HashMap.keySet():

The set supports element removal, which removes the corresponding mapping from the map, via the Iterator.remove, Set.remove, removeAll, retainAll, and clear operations. It does not support the add or addAll operations.

This is even specified within the Map interface itself.

---

¹ I decided to edit my answer rather than just commenting on psanton's, as I figured the extra information I'd got for similar-but-distinct situations was sufficiently useful to merit this answer staying.

Answer 2

you should use the entry set:

for(Iterator<Map.Entry<String, Node>> it = map.entrySet().iterator(); it.hasNext();)
{
      Map.Entry<String, Node> entry = it.next();
      Node n = entry.getValue().optimize();
      if(n == null) 
          it.remove();
      else
          entry.setValue(n);
}

EDIT fixed code