http://gwt-code-reviews.appspot.com/636801/diff/1/4
File /bikeshed/src/com/google/gwt/collections/MutableSet.java (right):
http://gwt-code-reviews.appspot.com/636801/diff/1/4#newcode23
/bikeshed/src/com/google/gwt/collections/MutableSet.java:23: public
final class MutableSet<E> extends Set<E> {
On 2010/06/17 19:22:07, Dan Rice wrote:
Need to document the null behavior of each method
Done.
http://gwt-code-reviews.appspot.com/636801/diff/1/4#newcode58
/bikeshed/src/com/google/gwt/collections/MutableSet.java:58: public
boolean containsAll(Set<E> source) {
On 2010/06/17 19:22:07, Dan Rice wrote:
Assert or check source != null (also other methods that take a source
param)
Done.
http://gwt-code-reviews.appspot.com/636801/diff/1/4#newcode83
/bikeshed/src/com/google/gwt/collections/MutableSet.java:83: public
boolean isEqual(Set<E> source) {
On 2010/06/17 19:22:07, Dan Rice wrote:
Faster approach: first check source.size == this.size, then do a
single
containsAll()
Also, need to check source != null before you do anything else
Done.
http://gwt-code-reviews.appspot.com/636801/diff/1/4#newcode107
/bikeshed/src/com/google/gwt/collections/MutableSet.java:107:
elements.remove(adapt(element));
On 2010/06/17 19:22:07, Dan Rice wrote:
Check if adapt(element) == null as you do in add(element)
Done.
http://gwt-code-reviews.appspot.com/636801/diff/1/6
File /bikeshed/src/com/google/gwt/collections/Set.java (right):
http://gwt-code-reviews.appspot.com/636801/diff/1/6#newcode85
/bikeshed/src/com/google/gwt/collections/Set.java:85: final String
adapt(Object value) {
On 2010/06/17 19:22:07, Dan Rice wrote:
This could also be done by initializing with a defailt adapter
protected Relation<Object, String> adapter = new Relation<Object,
String>() {
String adapt(Object value) {
return (String) value;
}
}
and ensure the logic for setAdapter allows the default adapter to be
replaced.
This avoids if tests every time
Done.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003
File /bikeshed/src/com/google/gwt/collections/MutableSet.java (right):
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode23
/bikeshed/src/com/google/gwt/collections/MutableSet.java:23: * creating
the Set with {...@link CollectionFactory#createMutableSet(Relation)}.
On 2010/06/21 18:16:23, Dan Rice wrote:
Document that the index strings must be unique.
Document that each Set can have a different adapter with a different
mapping
from elements to index strings.
Done.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode35
/bikeshed/src/com/google/gwt/collections/MutableSet.java:35: * if
(value == null || !(value instanceof Foo) ) {
On 2010/06/21 18:16:23, Dan Rice wrote:
'null' always fails an instanceof check -- so it is safe to say:
if (!(value instanceof Foo)) { // includes value == null
A nicer way to write the code might be:
return (value instanceof Foo) ? ((Foo) value).getIndex() : null;
Done.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode45
/bikeshed/src/com/google/gwt/collections/MutableSet.java:45: * {...@link
CollectionFactory#createMutableSet()}. This adapter provides uses
On 2010/06/21 18:16:23, Dan Rice wrote:
Remove 'provides' (if I understand your intent correctly)
Done.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode48
/bikeshed/src/com/google/gwt/collections/MutableSet.java:48: *
<code>null</code>.)
On 2010/06/21 18:16:23, Dan Rice wrote:
. outside )
Done.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode55
/bikeshed/src/com/google/gwt/collections/MutableSet.java:55: * Adds an
{...@code element} to this set. {...@code element} must be a value
On 2010/06/21 18:16:23, Dan Rice wrote:
I rarely see {...@code} in GWT sources -- if you do use it, I suggest
just tagging
the first use of a given symbol.
Also, when referencing a method you can use {...@link} instead so you
will have not
only code style but an active link as well. Be sure to use a # to
separate the
class and method names.
This made me remember reading about this in coding style guides, so I
went and looked around.
The java style in Google and elsewhere indicates the use of {...@code} tags
to visually format code snippets, keywords and such, not just the first
instance.
I also like using {...@link} but using it too much seems to be frowned
upon, with {...@code} being preferred after the first {...@link}. See
http://www.corp.google.com/~codewalk/12888859/ for example. {...@code} is
even preferred to <code> as the former works well with '<' and '>' while
the latter requires using < and >.
A quick grep in trunk (outside bikeshed) would seem to indicate {...@code}
usage has been picking up lately (79 files, 1092+ instances).
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode59
/bikeshed/src/com/google/gwt/collections/MutableSet.java:59: * @param
element element to add.
On 2010/06/21 18:16:23, Dan Rice wrote:
No . following a @param description. Please check for this in each
method.
Done. I'll correct the files outside this patch separately.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode76
/bikeshed/src/com/google/gwt/collections/MutableSet.java:76: throw new
NullPointerException();
On 2010/06/21 18:16:23, Dan Rice wrote:
Add a message for the NPE like "source == null".
Done.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode129
/bikeshed/src/com/google/gwt/collections/MutableSet.java:129: &&
source.elements.keySet().size() == elements.keySet().size()
On 2010/06/21 18:16:23, Dan Rice wrote:
HashMap has a size() method -- so no need to call keySet() just to get
the size.
Done.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode145
/bikeshed/src/com/google/gwt/collections/MutableSet.java:145: for (E e :
elements.values()) {
On 2010/06/21 18:16:23, Dan Rice wrote:
I'm a bit worried about whether this:
a) forces construction of an entire set of all elements; or
b) can fail due to a ConcurrentModificationException
For those reasons, I would prefer an iterator-based approach for these
methods.
Bit I can live with this approach since it is dev mode only.
Done.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode164
/bikeshed/src/com/google/gwt/collections/MutableSet.java:164:
elements.remove(adapt(element));
On 2010/06/21 18:16:23, Dan Rice wrote:
adapt(element) -> key
Done.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7003#newcode188
/bikeshed/src/com/google/gwt/collections/MutableSet.java:188: *
@exception NullPointerException if {...@code adapter} is <code>null</code>.
On 2010/06/21 18:16:23, Dan Rice wrote:
Maybe IllegalStateException if there is already an adapter?
Done. Also made imEmpty assertion into an IllegalStateException
exception
http://gwt-code-reviews.appspot.com/636801/diff/6001/7007
File
/bikeshed/test/com/google/gwt/collections/MutableSetAdapterTest.java
(right):
http://gwt-code-reviews.appspot.com/636801/diff/6001/7007#newcode114
/bikeshed/test/com/google/gwt/collections/MutableSetAdapterTest.java:114:
assertTrue(true);
On 2010/06/21 18:16:23, Dan Rice wrote:
I don't see the point of calling assertTrue(true) -- a comment should
be
sufficient
Done.
http://gwt-code-reviews.appspot.com/636801/diff/6001/7007#newcode162
/bikeshed/test/com/google/gwt/collections/MutableSetAdapterTest.java:162:
assertFalse(msA.isEqual(null));
On 2010/06/21 18:16:23, Dan Rice wrote:
How about assertFalse(msA.isEqual(new Object())
That will not compile. Set#isEqual(Set<E>) is not applicable to
Set#isEqual(Object). Object equality (Set#equals(Object)) would have
been better, but Overlay Types prevent overriding equals(Object). So I
felt a separate isEquals() was necessary. Should I rename isEquals to
something else, like containsSame, to prevent confusion?
http://gwt-code-reviews.appspot.com/636801/diff/6001/7008
File /bikeshed/test/com/google/gwt/collections/MutableSetTest.java
(right):
http://gwt-code-reviews.appspot.com/636801/diff/6001/7008#newcode210
/bikeshed/test/com/google/gwt/collections/MutableSetTest.java:210:
assertTrue(msC.isEqual(msA));
On 2010/06/21 18:16:23, Dan Rice wrote:
Redundant lines 210 and 211, did you mean something different?
Removed.
http://gwt-code-reviews.appspot.com/636801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
