RFR: String.join(), StringJoiner additions

[Alan Bateman]

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