Going through each.
On Thu, Mar 3, 2011 at 3:59 AM, Stephen Colebourne <scolebou...@joda.org> wrote:
> I'm not overly enthused about some of the changes, but since I've not
> been paying attention its difficult for me to vote/block. Anyway here
> is my review:
>
> ArrayUtils.hashCode() has been removed, but it had different
> functionality to Arrays.hashCode wrt nested arrays.
>
> Object[] arrayA = new Object[] {new boolean[] {true, false},
> new int[] {6, 7}};
> Object[] arrayB = new Object[] {new boolean[] {true, false},
> new int[] {6, 7}};
> assertEquals(true, Arrays.hashCode(arrayB) == Arrays.hashCode(arrayA));
Fair. Should be brought back in.
> I don't love the new Pair class. We have an interface based version
> here at OpenGamma to allow primitive implementations for performance.
> I might be able to get our code released if there was interest.
Tbh, ENOCARE :) I'm happy to see there's good discussion, but I only
ever needed one when porting Scheme tutorials to Java for amusement.
> ArrayUtils.toArray() javadoc has example code that won't compile
> (missing "new") and also misses < > in places.
Jörg fixed.
> Some new methods use a different brace position from the existing
> code, such as ArrayUtils.toArray(), Validate.matchesPattern(),
> Validate.exclusiveBetween()...
Jörg fixed.
> Some new code isn't explicit about null handling, such as
> AnnotationUtils, CharSequenceUtils, Validate.exclusiveBetween()...
Do you mean that the javadoc doesn't explicitly say what happens when
null is passed in?
> Some new methods don't have an @since, such as Validate.notBlank(),
> Validate.exclusiveBetween()...
Note; action item to go through the hack-Clirr and cross-reference
missing @sinces.
> StringUtils is now an odd mixture of methods that accept a
> CharSequence and ones that don't. Looking at it, I'd prefer to see
> CharSequenceUtils added to, and StringUtils methods just deal with
> Strings (A CharSequence might be mutable, so it has a different set of
> implications when writing code using it). But if that isn't OK, then
> it would be better to see everything in StringUtils take a
> CharSequence.
By design :(
The aim is that everything in StringUtils that can reasonably take a
CharSequence does; but to move a method to CharSequenceUtils simply
because we made it use a higher interface is silly.
Given the 1 method in CharSequenceUtils, discussing this makes me tend
towards shoving it in the bloated StringUtils class.
> ToStringStyle doesn't have a serialization version ID.
Presumably hasn't had one for a long time :) It's an abstract class -
it feels to me that it doesn't need to have a serialization version
ID.
> While we've moved away from NullArgumentException, I suspect it may be
> reasonably widely used.
People can make the case to add it back in :) Having Exceptions that
weren't related to our actual API turned into an exercise in navel
gazing.
> DateUtils has added a new MODIFY int enum, rather than using a real
> enum. Nor has the existing RANGE int enum been converted
Good point.
I'm still very tempted to pull the entire package. It would be fun to
port/submit the code into Joda Time and see what you parts you find
acceptable there.
> The JavaVersion name field is not used.
It is in spirit :) Pretend there's a map there with the name as the
key. Then pretend that I unrolled the map to have a simple String case
statement :)
Or ignore that and I should probably add a toString that will make it used.
> ObjectUtils.firstNonNull() differs from Google Guava in that it can
> return null. This is probably OK, but should be checked.
Six of one, half a dozen of the other. I'm not sure if returning null
or throwing NPE has any specific value over the other.
> Range is only thread safe if the objects held inside are thread safe
> (javadoc).
Added.
> Range.containsRange() might be better named containsAll()
> Range.overlapsRange might be better named overlaps()
I've gone with containsAll() and overlapsWith().
> Public constants on StringEscapeUtils do not have javadoc.
Reported as LANG-682 (as I have to do work now).
> The StringUtils.concat methods duplicate the existing join methods.
LANG-683.
> There are a number of TODOs in the code that might need addressing.
I've gone through these and none have been critical. Please feel free
to call them out if they are (the website has a todo report).
> The migration guide exceptions section is missing a header.
Fixed.
> Hope some of this helps (!)
Very much so. Stick around a bit more :)
Hen
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
