On 12/04/2013 16:02, Alan Bateman wrote:
On 11/04/2013 23:33, Jim Gish wrote:
Please review
http://cr.openjdk.java.net/~jgish/Bugs-5015163-7175206-7172553/
<http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7175206-7172553/>
These are changes that we made in lambda that we're now bringing into
JDK8.
I've made a couple of additions - making StringJoiner final and
adding a couple of constructors to set the emptyOutput chars.
Thanks,
Jim
:
I plan to look at StringJoiner in more detail later.
Just to follow up with a few comments on StringJoiner.
I don't know how "final" this is but I wonder if you've already
experimented (and rejected) having a smaller set of constructors? I will
guess that the most popular usage will be the simple 1-arg constructor
to just specify the delimiter. There will likely be some cases where you
want a prefix and suffix too. I bring this up because it seems a bit
inconsistent to just have a setter for the default result when one could
as easily have a method to set the prefix/suffix too. Clearly it would
complicate the implementation a bit but it could be optimized for the
case that these are set before any elements are added. Anyway, I'm not
trying to re-open discussions on this, just trying to understand if what
you are proposing is already close to final.
On method names then "setEmptyOutput" doesn't seem quite right, I wonder
if you've considered others, like setEmptyValue or setDefaultResult.
Minor nits:
- The javadoc for "add" starts with "add the supplied", should be "Add".
- The @param in the 1-arg constructor is indented inconsistently to the
other methods
- The this(...) usage in the 3-arg constructor has spaces around it so
it is inconsistent with the other usages.
- In the class description it reads "Prior to adding something to the
StringJoiner, {@code sj.toString()} will, by default, return {@code
prefix+suffix}". This might be better as "Prior to adding elements to a
StringJoiner, its toString() method, if invoked, will ...".
- The comma in "For example," set expectations that there will be more
text after the example but this isn't so.
- As with the comments on String.join then I assume the test should have
1 bug number (not 3). Also to be consistent with the existing
organization it would be better to move it down to
test/java/util/StringJoiner (I know we have to come up with a solution
for tests with package names).
- The test has two @summary lines, I assume this is a mistake.
- In terms of code coverage then it looks like the only method that is
tested for NPE is setEmptyOutput.
That's all I have.
-Alan
