On Jul 3, 2013, at 9:09 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> On 03/07/2013 09:03, Henry Jen wrote: >> Hi, >> >> Please review a simple addition of StringJoiner.merge method. This is >> useful to combine StringJoiners used in parallel doing joining works. >> >> The webrev can be found at >> http://cr.openjdk.java.net/~henryjen/ccc/8017231.0/webrev/ >> >> Also included is a little clean up for StringJoinerTest. >> >> Cheers, >> Henry > Is this named "merge" to disambiguate it from "add"? Just wondering. > I think it's a better reflection on the fact of "merge" of elements like collections, instead of feels like add(StringJoiner.toString()). > A minor point but I think the wider javadoc tends to use "the given XYZ" when > referring to parameters rather than the "the supplied XYZ". > > Typo "nonempty" -> "non-empty". > Will fix those. > I assume you meant to name the local otherBuilder rather than an otherBuffer. > Also is there any reason not to use append(CharSequence,int,int) here? > Will fix the naming as well. As for append, I don't think there is a particular reason. It would be best if we can do append(char[], start, end), but I don't see there is any way to do that. I suspect append(CS, int, int) should be a little faster without increasing the count each time give a check of range. I'll update it to use append(CharSequence, int, int). > The test looks good to me, I guess an alternative would be to just add to the > existing test so that we have one test for StringJoiner (it doesn't matter > either way). > I'll leave it out, I think it is easier to navigate that way. Do we need another round for above updates? Cheers, Henry
