2

This a a similar question to [FindBugs warning: Inefficient use of keySet iterator instead of entrySet iterator

However, there I am trying to do something a little different. My current code is here: 

for (Double key2 : sortedPolygons.keySet()) {
    if (sortedPolygons.get(key2).getExteriorRing().equals(hole)) {
        sortedPolygons.remove(key2);
        break;
    }
}

Doing something like the solution in the link does not work. Here is an implementation of said solution:

for(Map.Entry<Double, Polygon> entry : sortedPolygons.entrySet()) {
    if (entry.getValue().getExteriorRing().equals(hole)) {
         .....

The problem here is that I am trying to delete the entry. There is no entry.remove(). How can I replace my first block of code, without the FindBugs error:

Inefficient use of keySet iterator instead of entrySet iterator ->

This method accesses the value of a Map entry, using a key that was retrieved from a keySet iterator. It is more efficient to use an iterator on the entrySet of the map, to avoid the Map.get(key) lookup.

To note, the underlying structure is TreeMap, and it cannot be changed.

Community
  • 1
  • 1
DanGordon
  • 671
  • 3
  • 8
  • 26

2 Answers2

5

I fail to understand your reasoning: in the first snippet, you use 

sortedPolygons.remove(key2);

to remove a key. Nothing prevents you to do the same in the second snippet:

sortedPolygons.remove(entry.getKey());

Whatever the way you iterate, this will lead to a ConcurrentModificationException anyway, because for most collections, you can't modify it while iterating on it, except by using its iterator.

Quote from the javadoc:

The iterators returned by the iterator method of the collections returned by all of this class's "collection view methods" are fail-fast: if the map is structurally modified at any time after the iterator is created, in any way except through the iterator's own remove method, the iterator will throw a ConcurrentModificationException.

So the code should be:

for (Iterator<Map.Entry<Double, Polygon>> it = sortedPolygons.entrySet().iterator(); it.hasNext(); ) {
    Map.Entry<Double, Polygon> entry = it.next();
    if (entry.getValue().getExteriorRing().equals(hole)) {
        it.remove();
        // if you want to exit the loop as soon as you found a match:
        break;
    }
}
JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
  • 1
    Note: the key isn't actually needed (see my answer) – Peter Lawrey Jul 07 '14 at 20:42
  • 1
    @PeterLawrey: agreed. +1 to your answer. I'll leave my answer as is, to show the OP how to iterate over entries, just in case he needs the key in the future. – JB Nizet Jul 07 '14 at 20:44
  • The OP was `break`ing after the `remove`. But +1 for using an `Iterator`. – Boris the Spider Jul 07 '14 at 20:45
  • @BoristheSpider: Now I see it. That's why he didn't get a ConcurrentModificationException. Very fragile though. I would still use an Iterator, which is also more efficient at removing anyway. – JB Nizet Jul 07 '14 at 20:47
  • `sortedPolygons.remove(entry.getKey());` somehow didn't think of doing that, but is there a reason you don't use a for-each loop? Isn't there a way to do that in this situation? And is there a reason you removed the `break;`? – DanGordon Jul 07 '14 at 20:48
  • @DanGordon: If you want to break as soon as you found a matching value, then the break should be there. I didn't use the foreach loop because, without the break, that would cause a ConcurrentModificationException, as explained in the answer. The only safe way to remove elements while iterating is to remove the elements using the iterator. The foreach loop doesn't allow that. – JB Nizet Jul 07 '14 at 21:00
  • 1
    It’s worth noting that you don’t need to use the `entrySet()` for that; every collection view supports removal. So here, you could use `for(Iterator it = sortedPolygons.values() .iterator(); it.hasNext(); ) if(it.next().getExteriorRing().equals(hole)) { it.remove(); break; }` If no short-circuiting is required, Java 8’s `sortedPolygons.values() .removeIf(poly -> poly.getExteriorRing().equals(hole));` would be even smoother. – Holger Jun 08 '21 at 15:06
4

How about you use the entrySet() iterator as suggested.

for(Iterator<Map.Entry<Double, Ploygon>> iter = sortedPolygons.entrySet().iterator(); 
         iter.hasNext();) {
    Map.Entry<Double, Ploygon> entry = iter.next();

    if (condition)
        iter.remove();
}

However you don't need the key so you can just iterate the values

for(Iterator<Ploygon> iter = sortedPolygons.values().iterator(); 
         iter.hasNext();) {
    Ploygon ploygon = iter.next();

    if (condition)
        iter.remove();
}
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130