On 11/24/15 7:03 AM, Remi Forax wrote:
The other way to detect collision instead of checking the size if to use the
return value of put, it should be null if it's a new key,
but the code is maybe less readable.
Yes. This is just a "skeleton" implementation, so I'm not terribly interested in
making a bunch of improvements to the code. But if this code does end up
surviving into the final release, we should definitely take a closer look at
some of these details.
There are several instance of
@SafeVarargs
@SuppressWarnings("varargs")
if you use @SafeVarargs you should not to use @SuppressWarnings("varargs").
These cover different cases. The @SafeVarargs annotation is a declaration to the
caller that this method doesn't cause heap pollution, so that callers of this
method don't get a warning.
The @SuppressWarnings("varargs") is to suppress a warning within this method at
the call to Arrays.asList(), which does issue a varargs warning. (This despite
the fact that Arrays.asList() is annotated with @SafeVarargs. Hmm.)
I should move the @SuppressWarnings to the right spot within the method, though,
as it doesn't need to apply to the entire method.
In Map.ofEntries, calling Objects.requireNonNull on the entry inside the for
loop is not necessary given getKey is called on that entry.
Yes, also noted by Paul Benedict.
For the Set, i think i prefer to avoid the call to Array.asList and use add
instead,
...
and Set.of(E...) can be simply written like this:
...
As above, I don't want to spend too much time on the implementation right now as
it's not the "real" implementation. The plan is to replace this entirely with
compacte, optimized implementations. That's where the micro-optimization reviews
should come in. :-)
For KeyValueHolder, in equals, you don't need to access to the getter here,
return key.equals(e.key) && value.equals(e.value);
In this case e is Map.Entry<?,?> which could be an instance of one of the other
Map.Entry implementations, not necessarily a KeyValueHolder, so this needs to
use getKey() and getValue().
and if you can fix something that baffle me with the usual implementations of
Map.Entry,
a good hashCode for KeyValueHolder should have the property that hashCode for
entry(a, b) is different form the hashCode of entry(b, a),
something like:
return key.hashCode() ^ Integer.rotateLeft(value.hashCode(), 16);
or any variations.
Sigh. This hashCode computation is specified in Map.Entry.hashCode() and is
implemented this way by AbstractMap.SimpleEntry/SimpleImmutableEntry. I think
KeyValueHolder should match. Sorry.
(Note also that KeyValueHolder is no longer a public class.)
s'marks
