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