On 17/04/2013 22:49, Jim Gish wrote:
Here's an update:
http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/
<http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/>
Jim
StringJoiner looks much better now, good to see it reduced down to 2
simple constructors.
One thing that I didn't bring up in the previous round is that as all
the parameters are CharSequences, then it probably should be made clear
that the constructors and setEmptyValue method take copies. I'm not
suggesting it would be logical to implement it otherwise but if someone
using StringBuilder or mutable CharSequences implementation needs to be
sure that StringJoiner will do the right thing if the char sequence is
changed subsequently.
In the class description it reads:
"if the {@code emptyValue} parameter is supplied via the ..."
which is a bit confusing because a StringJoiner doesn't have a
"emptyValue parameter". It might be clearer if changed to:
"if a default <i>empty value</i> is specified via the ..."
A minor comment on the wording in the constructors is that "Also" should
probably be dropped from the second sentence.
The 3-arg constructor still specifies that it throws NPE if the
emptyValue is null but there isn't an emptyValue parameter here.
In toString then I assume that you can avoid the +suffix when it is the
empty string (which is going to very common).
You've addressed my previous comment on String.join not specifying how
null behaves so this is clear now - thanks.
Also looks like you've moved the tests and extended the coverage for
nulls - thanks.
So overall I think this is looking much better.
-Alan.
