CODEWITHSUNDEEP
javacollectionsdictionaryawt
Aubin
Aubin

Reputation: 14853

java Map removal unexpected behavior

code:

public class FeatureBuilder implements IFeatureBuilder {

   private final Map< java.awt.Shape, ShapeAttributes > shapes = new HashMap<>();

   [...]

   @Override
   public void addToContainer( java.awt.Shape shape ) {
      System.err.println( shape.getClass());
      shapes.put( shape, new ShapeAttributes( ... ));
   }

   @Override
   public void removeFromContainer( double x, double y ) {
      final Set< Shape > rm  = new HashSet<>();
      final Point2D      loc = new Point2D.Double( x, y );
      for( final Shape shape : shapes.keySet()) {
         if( shape.contains( loc )) {
            System.err.println( "shapes.containsKey( shape ): " +
               shapes.containsKey( shape ));
            rm.add( shape );
         }
      }
      System.err.println( "cardinality before removal : " + shapes.size());
      shapes.keySet().removeAll( rm );
      System.err.println( "cardinality after  removal : " + shapes.size());
   }

output:

class java.awt.geom.Rectangle2D$Double
shapes.containsKey( shape ): false      <<<<<<< This is unexpected!
cardinality before removal : 1
rm cardinality             : 1
cardinality after  removal : 1

I'm surprised: an instance retrieved by a for iterator on Map.keySet() is not a key in the Map!

How is this possible?

The main bug in this method is the selected instances of Shape placed in rm are not removed from shapes.

After reading your answers the code becomes:

public class DecoratedShape {

   public final Shape _shape;
   public /* */ Color _stroke      = Color.BLACK;
   public /* */ float _strokeWidth = 3.0f;
   public /* */ Color _fill        = Color.BLACK;

   public DecoratedShape( Shape shape, Color stroke, float strokeWidth, Color fill ) {
      _shape       = shape;
      _stroke      = stroke;
      _strokeWidth = strokeWidth;
      _fill        = fill;
   }

   public boolean contains( Point2D loc ) {
      return _shape.contains( loc );
   }

   public void paint( Graphics2D g ) {
      g.setStroke( new BasicStroke( _strokeWidth ));
      g.setColor( _fill );
      g.fill( _shape );
      g.setColor( _stroke );
      g.draw( _shape );
   }
}

public class FeatureBuilder implements IFeatureBuilder {

   private final List< DecoratedShape > _shapes = new LinkedList<>();
   [...]

   @Override
   public void addToContainer( Object o ) {
      _shapes.add( new DecoratedShape((Shape)o, _stroke, _strokeWidth, _fill ));
   }

   @Override
   public void removeFromContainer( double x, double y ) {
      final Set<DecoratedShape> rm = new HashSet<>();
      final Point2D loc = new Point2D.Double( x, y );
      for( final DecoratedShape shape : _shapes ){
         if( shape.contains( loc ) ){
            rm.add( shape );
         }
      }
      _shapes.removeAll( rm );
   }

   public void paint( Graphics2D g ) {
      for( final DecoratedShape shape : _shapes ) {
         shape.paint( g );
      }
   }
}

And now, it works as expected... Thanks a lot!

Upvotes: 4

Views: 109

Answers (2)

assylias
assylias

Reputation: 328598

Following-up on @RohitJain's comment, here is a simple example that reproduces the behaviour:

Map<Shape, Object> map = new HashMap<>();
java.awt.geom.Rectangle2D.Double shape = new java.awt.geom.Rectangle2D.Double(1, 1, 1, 1);
map.put(shape, null);
System.out.println(map.size()); //1
shape.setRect(2, 2, 2, 2); //mutate
System.out.println(map.size()); //still 1
map.keySet().remove(shape);
System.out.println(map.size()); //still 1

The underlying problem is that the Shape is mutated while in the map.

Upvotes: 4

Njol
Njol

Reputation: 3279

This doesn't explain why your code doesn't work, but it will fix your issue and make the method more efficient as no temporary set is allocated:

public void removeFromContainer(double x, double y) {
    final Point2D loc = new Point2D.Double(x, y);
    Iterator<Shape> iter = shapes.keySet().iterator();
    while (iter.hasNext()) {
        if (iter.next().contains(loc))
            iter.remove();
    }
}

edit: I suspect that you modify the Shape(s) after you add them to the map, so that the hash code when checking for containsKey doesn't match the hash code used when adding the Shape to the map.

Upvotes: 2

Related Questions