RFR: String.join(), StringJoiner additions (original) (raw)
Alan Bateman Alan.Bateman at oracle.com
Mon Apr 15 17:46:30 UTC 2013
- Previous message: RFR: String.join(), StringJoiner additions
- Next message: RFR: String.join(), StringJoiner additions
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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
- Previous message: RFR: String.join(), StringJoiner additions
- Next message: RFR: String.join(), StringJoiner additions
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]