Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(309)

Issue 107058: Some utility classes that mirror com.google.common.collections. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
16 years, 7 months ago by MikeSamuel
Modified:
16 years, 7 months ago
Reviewers:
DavidSarah, MarkM
CC:
google-caja-discuss_googlegroups.com
Base URL:
http://google-caja.googlecode.com/svn/trunk/
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 21

Patch Set 2 : Some utility classes that mirror com.google.common.collections. #

Total comments: 1

Patch Set 3 : Some utility classes that mirror com.google.common.collections. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+701 lines, -0 lines) Patch
A src/com/google/caja/util/Lists.java View 1 2 1 chunk +78 lines, -0 lines 1 comment Download
A src/com/google/caja/util/Maps.java View 1 1 chunk +86 lines, -0 lines 0 comments Download
A src/com/google/caja/util/Multimap.java View 1 1 chunk +40 lines, -0 lines 2 comments Download
A src/com/google/caja/util/Multimaps.java View 1 1 chunk +170 lines, -0 lines 0 comments Download
A src/com/google/caja/util/Sets.java View 1 1 chunk +144 lines, -0 lines 1 comment Download
M tests/com/google/caja/AllTests.java View 2 chunks +2 lines, -0 lines 0 comments Download
A tests/com/google/caja/util/CollectionsTest.java View 1 chunk +181 lines, -0 lines 0 comments Download

Messages

Total messages: 6
MikeSamuel
16 years, 7 months ago (2009-09-02 02:25:01 UTC) #1
MarkM
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 ...
16 years, 7 months ago (2009-09-02 03:32:36 UTC) #2
MikeSamuel
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, ...
16 years, 7 months ago (2009-09-02 18:59:20 UTC) #3
MikeSamuel
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 ...
16 years, 7 months ago (2009-09-02 19:13:47 UTC) #4
MarkM
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 ...
16 years, 7 months ago (2009-09-02 19:18:04 UTC) #5
DavidSarah
16 years, 7 months ago (2009-09-02 22:22:34 UTC) #6
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).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b