maidh91 / guava-libraries

Automatically exported from code.google.com/p/guava-libraries
Apache License 2.0
0 stars 0 forks source link

ImmutableMap.Builder.put() should throw an IllegalArgumentExcepotion for duplicate keys #360

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Maps do not allow duplicate keys. ImmutableMap.Builder does allow duplicate
keys. That is, you can put as many duplicate keys in the map as you like,
and no exception is thrown. However an IllegalArgumentException is thrown
when you attempt to build() an ImmutableMap from the builder. 

There is nothing one can do with an ImmutableMap.Builder that contains
duplicate keys. You can't inspect it, and you can't build a map from it.
You can't even remove the duplicate key. As soon as the duplicate key is
inserted, the builder is irretrievably broken.

Instead of allowing a duplicate key to be inserted, the put method should
throw an IllegalArgumentException. This is a runtime exception and a
compatible change. This has two beneficial effects:

1. It makes it much easier to locate and debug problems. Currently, you
only become aware of the problem when build() is called, possibly long
after the root cause of duplicate key insertion. It helps to know where and
when the duplicate key is inserted, not merely that someone, somewhere, put
in a duplicate key.

2. It makes it possible to recover from duplicate key insertion by catching
the IllegalArgumentException. Currently this is unrecoverable. You have to
rebuild the map from scratch at best, and even that is difficult because
you can't tell what key caused the problem. Enabling the programmer to
(optionally) catch the exception at put time instead of build time gives
them the chance to ignore the exception if, for example, they just need to
make sure that the key has some value and don't care which of several
values they assign to it. 

In my case I was making a map of timezones to cities. I needed one city per
timezone, but I didn't care which city it was. As it happened, I had
multiple cities (values) in some timezones (keys) and consequently the
builder blew up. Debugging was complicated because the step that blew up
was not the step with the problem Fixing this required an extra deduping
step on my keys. 

Original issue reported on code.google.com by erhar...@gmail.com on 12 May 2010 at 1:31

GoogleCodeExporter commented 9 years ago
As a workaround, you can use a HashMap as your builder. Then you can detect 
dupes, if 
necessary, by examining the value that map.put() returns. Call 
ImmutableMap.copyOf() 
to generate the ImmutableMap.

Original comment by jl...@google.com on 12 May 2010 at 6:15

GoogleCodeExporter commented 9 years ago

Original comment by kevinb@google.com on 30 Jul 2010 at 3:56

GoogleCodeExporter commented 9 years ago

Original comment by fry@google.com on 28 Jan 2011 at 4:13

GoogleCodeExporter commented 9 years ago

Original comment by kevinb@google.com on 13 Jul 2011 at 6:18

GoogleCodeExporter commented 9 years ago

Original comment by fry@google.com on 10 Dec 2011 at 3:46

GoogleCodeExporter commented 9 years ago

Original comment by fry@google.com on 16 Feb 2012 at 7:17

GoogleCodeExporter commented 9 years ago

Original comment by kevinb@google.com on 22 Jun 2012 at 6:16

GoogleCodeExporter commented 9 years ago
This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:15

GoogleCodeExporter commented 9 years ago

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:18

GoogleCodeExporter commented 9 years ago

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:10