Some utility classes that mirror com.google.common.collections.

Utility classes for Maps, Sets, Lists, and Multimaps that were inspired by
http://code.google.com/p/google-collections/ but which don't use any
@SuppressWarning("generic") annotations.


Submitted @3685

[MarkM, 2009-09-02 03:32:36]

http://codereview.appspot.com/107058/diff/1/6
File src/com/google/caja/util/Lists.java (right):

http://codereview.appspot.com/107058/diff/1/6#newcode23
Line 23: * Shortcuts for creating maps.
s/maps/lists/

http://codereview.appspot.com/107058/diff/1/6#newcode28
Line 28: public final class Lists {
Why no var_args overloads like you did for newHashSet? Seems like it would be
equally convenient here.

http://codereview.appspot.com/107058/diff/1/2
File src/com/google/caja/util/Maps.java (right):

http://codereview.appspot.com/107058/diff/1/2#newcode53
Line 53: return new LinkedHashMap<K, V>();
s/LinkedHashMap/IdentityHashMap/

http://codereview.appspot.com/107058/diff/1/2#newcode58
Line 58: return new LinkedHashMap<K, V>(map);
s/LinkedHashMap/IdentityHashMap/

http://codereview.appspot.com/107058/diff/1/3
File src/com/google/caja/util/Multimaps.java (right):

http://codereview.appspot.com/107058/diff/1/3#newcode36
Line 36: public class Multimaps {
Since your Maps preserves the distinction between the Map and SortedMap
interfaces, should we also have a SortedMultiMap interface to preserve the
parallelism. (If the answer is that we don't need it now, a note that it would
be a logical future extension is adequate.)

http://codereview.appspot.com/107058/diff/1/3#newcode38
Line 38: Multimap<K, V> newListHashMultimap() {
For this method and all of its siblings, if you can add a succint doc-comment
that says what it does or what it is for or when you might want to use this one
-- as distinct from how it is implemented -- that'd be great.

Btw, I do love how you've factored the implementation.

http://codereview.appspot.com/107058/diff/1/3#newcode68
Line 68: this.underlying = underlying;
Please state the invariant that the underlying only stores associations whose
values are non-empty collections.

http://codereview.appspot.com/107058/diff/1/3#newcode95
Line 95: return c.add(v);
According to the
http://java.sun.com/j2se/1.5.0/docs/api/java/util/Collection.html#add%28E%29
contract, the underlying can refuse to add v even if it is not already present
if it reports this failure with an exception. In that case, your put method
leaves the underlying in violation of its invariant. If you have some reason to
believe that this case can't occur for these adds, you should state it.

http://codereview.appspot.com/107058/diff/1/3#newcode106
Line 106: c.addAll(v);
Same issue if addAll exits abruptly. You could fix this by putting the next line
inside a finally, but that's worrisome. Although the code would look more
awkward, I think both cases should be handled with a pattern like:

if (c == null) {
  c = makeCollection();
  boolean result = c.add[All](v);
  [if (c.isEmpty()) { return result; }]
  underlying.put(k, c);
  return result;
} else {
  return c.add[All](v);
}

Although ugly, I find it easier to think about than using a finally to repair
this problem.

http://codereview.appspot.com/107058/diff/1/4
File src/com/google/caja/util/Sets.java (right):

http://codereview.appspot.com/107058/diff/1/4#newcode62
Line 62: Set<E> newLinkedHashSet(Collection<? extends E> els) {
Why no var_args overload like you did for newHashSet? Seems equally convenient.
Likewise for the other static constructors below.

[MikeSamuel, 2009-09-02 18:59:20]

http://codereview.appspot.com/107058/diff/1/6
File src/com/google/caja/util/Lists.java (right):

http://codereview.appspot.com/107058/diff/1/6#newcode23
Line 23: * Shortcuts for creating maps.
On 2009/09/02 03:32:36, MarkM wrote:
> s/maps/lists/

Done.

http://codereview.appspot.com/107058/diff/1/6#newcode28
Line 28: public final class Lists {
On 2009/09/02 03:32:36, MarkM wrote:
> Why no var_args overloads like you did for newHashSet? Seems like it would be
> equally convenient here.

Done.

http://codereview.appspot.com/107058/diff/1/2
File src/com/google/caja/util/Maps.java (right):

http://codereview.appspot.com/107058/diff/1/2#newcode53
Line 53: return new LinkedHashMap<K, V>();
On 2009/09/02 03:32:36, MarkM wrote:
> s/LinkedHashMap/IdentityHashMap/

Ack.  Good catch.

http://codereview.appspot.com/107058/diff/1/2#newcode58
Line 58: return new LinkedHashMap<K, V>(map);
On 2009/09/02 03:32:36, MarkM wrote:
> s/LinkedHashMap/IdentityHashMap/

Done.

http://codereview.appspot.com/107058/diff/1/3
File src/com/google/caja/util/Multimaps.java (right):

http://codereview.appspot.com/107058/diff/1/3#newcode36
Line 36: public class Multimaps {
On 2009/09/02 03:32:36, MarkM wrote:
> Since your Maps preserves the distinction between the Map and SortedMap
> interfaces, should we also have a SortedMultiMap interface to preserve the
> parallelism. (If the answer is that we don't need it now, a note that it would
> be a logical future extension is adequate.)

I don't think it's necessary right now.
I don't know where to put such a note.

http://codereview.appspot.com/107058/diff/1/3#newcode38
Line 38: Multimap<K, V> newListHashMultimap() {
On 2009/09/02 03:32:36, MarkM wrote:
> For this method and all of its siblings, if you can add a succint doc-comment
> that says what it does or what it is for or when you might want to use this
one
> -- as distinct from how it is implemented -- that'd be great.
> 
> Btw, I do love how you've factored the implementation.

Done.

http://codereview.appspot.com/107058/diff/1/3#newcode68
Line 68: this.underlying = underlying;
On 2009/09/02 03:32:36, MarkM wrote:
> Please state the invariant that the underlying only stores associations whose
> values are non-empty collections.

Done.

http://codereview.appspot.com/107058/diff/1/3#newcode95
Line 95: return c.add(v);
On 2009/09/02 03:32:36, MarkM wrote:
> According to the
>
http://java.sun.com/j2se/1.5.0/docs/api/java/util/Collection.html#add%2528E%2529
> contract, the underlying can refuse to add v even if it is not already present
> if it reports this failure with an exception. In that case, your put method
> leaves the underlying in violation of its invariant. If you have some reason
to
> believe that this case can't occur for these adds, you should state it.

Done.

http://codereview.appspot.com/107058/diff/1/3#newcode106
Line 106: c.addAll(v);
On 2009/09/02 03:32:36, MarkM wrote:
> Same issue if addAll exits abruptly. You could fix this by putting the next
line
> inside a finally, but that's worrisome. Although the code would look more
> awkward, I think both cases should be handled with a pattern like:
> 
> if (c == null) {
>   c = makeCollection();
>   boolean result = c.add[All](v);
>   [if (c.isEmpty()) { return result; }]
>   underlying.put(k, c);
>   return result;
> } else {
>   return c.add[All](v);
> }
> 
> Although ugly, I find it easier to think about than using a finally to repair
> this problem.

Done.

http://codereview.appspot.com/107058/diff/1/4
File src/com/google/caja/util/Sets.java (right):

http://codereview.appspot.com/107058/diff/1/4#newcode62
Line 62: Set<E> newLinkedHashSet(Collection<? extends E> els) {
On 2009/09/02 03:32:36, MarkM wrote:
> Why no var_args overload like you did for newHashSet? Seems equally
convenient.
> Likewise for the other static constructors below.

Done.

[MikeSamuel, 2009-09-02 19:13:47]

http://codereview.appspot.com/107058/diff/1019/23
File src/com/google/caja/util/Lists.java (right):

http://codereview.appspot.com/107058/diff/1019/23#newcode54
Line 54: List<E> newLinkedList() {
Mark pointed out that Queue is not List.  I had thought that Queue extends List
but it does not.

I tried to make the signature
public static <E, C extends List<E>&Queue<E>>
but couldn't get it to return a value that could be used as either a list or
queue.

[MarkM, 2009-09-02 19:18:04]

LGTM. Neither comment below suggests any changes.

http://codereview.appspot.com/107058/diff/1/3
File src/com/google/caja/util/Multimaps.java (right):

http://codereview.appspot.com/107058/diff/1/3#newcode36
Line 36: public class Multimaps {
On 2009/09/02 18:59:20, MikeSamuel wrote:
> On 2009/09/02 03:32:36, MarkM wrote:
> > Since your Maps preserves the distinction between the Map and SortedMap
> > interfaces, should we also have a SortedMultiMap interface to preserve the
> > parallelism. (If the answer is that we don't need it now, a note that it
would
> > be a logical future extension is adequate.)
> 
> I don't think it's necessary right now.
> I don't know where to put such a note.

Not a big deal. Don't worry about it.

http://codereview.appspot.com/107058/diff/12/1010#newcode116
Line 116: return result;
For this one (after an add()), the isEmpty test isn't technically necessary.
(That's what the mysterious square brackets in my original comment were trying
to indicate.)

The other case, after an addAll(), it is necessary. Nevertheless, keep it as you
have it now. This bit of extra defensiveness is good.

[DavidSarah, 2009-09-02 22:22:34]

http://codereview.appspot.com/107058/diff/1019/22
File src/com/google/caja/util/Multimap.java (right):

http://codereview.appspot.com/107058/diff/1019/22#newcode28
Line 28: /**
code style: blank lines between methods

http://codereview.appspot.com/107058/diff/1019/22#newcode35
Line 35: void putAll(K k, Collection<? extends V> v);
all methods of interfaces should have a doc comment, even if it's fairly obvious

http://codereview.appspot.com/107058/diff/1019/21
File src/com/google/caja/util/Sets.java (right):

http://codereview.appspot.com/107058/diff/1019/21#newcode51
Line 51: Set<E> s = new HashSet<E>(els.length);
I think that that overriding methods to have both E... and non-varargs
signatures makes the meaning too ambiguous at the point of use. It's fine to
override based on, say, Collection vs Iterable, because there the parameter has
the same purpose in each override, but that's not the case when varargs are
thrown in. Consider giving all the E... variants a different name (not just in
this class).