commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Smith" <mich...@iammichael.org>
Subject [collections][PATCH] Various problems with AbstractBag
Date Sun, 10 Feb 2002 05:53:23 GMT
The following patch fixes the following problems with
org.apache.commons.collections.AbstractBag:

 - Documentation does not specify exactly what a subcless needs to do to
extend
AbstractBag to make a concrete subclass.

 - AbstractBag.add(Object,int) has two calls to getCount(o), when only
one is
necessary.  This wastes a few cycles to perform method invocations, a
map
lookup, a cast, and a few comparisons.

 - The equals(Object) method will incorrectly throw a
NullPointerException if a
null value is passed.  The Object.equals(Object) API specifies "For any
non-
null reference value x, x.equals(null) should return false".

 - The equals(Object) method will only work if the object passed in
extends
AbstractMap or implements Map.  Neither of these facts is documented,
and
neither is correct.  The contract for Object.equals(Object) states: "for
any
reference values x and y, x.equals(y) should return true if and only if
y.equals
(x) returns true. ".  Returning true when the argument is a Map is
incorrect
since he reverse (the map checking to see if its equal to the bag) will
most
certainly be false.  The same can be said for AbstractMap.  A subclass
of
AbstractMap may add extra data to be stored within the Bag that must
also be
compared for them to be equal.  The reverse comparison (specialized
subclass
equals basic abstract bap) will fail.  You can read more about this in a
three-
year old, but still valid java world article:
http://www.javaworld.com/javaworld/jw-01-1999/jw-01-object.html

 - if remove(Object,int) is called passing in 0 as the number to remove
and
specifying an object that exists in th bag, true will be returned from
the
method. Per the Bag API specification, true should only return when an
object
is actually removed.  Since no objects are removed, false should be
returned
instead. Additionally, if a negative number is specified, not only is
the
object not removed, but object(s) are *added* (well, in the sense that
it is
equivalent of calling add(o, -i))

 - the uniqueSet() method returns the set of unique objects, however the
set is
modifiable.  If the underlying map implementation has the set backed by
the map
(as per the map contract -- so it should), then elements can be removed
form
the unique set and have them removed from the underlying map as well.
This
causes consistency problems with the Bag since _total will then be
incorrect.

 - in extractList(), getCount(current) is called each time through the
inner
loop, adding lots of extra overhead.  Reversing the loop (starting at
the count
and going down to 0) eliminates the excess overhead.

Regards,
michael

Mime
View raw message