RFR: String.join(), StringJoiner additions
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 -- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
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
One question on the bug numbers. Are you proposing to use all three? I assume that at least 7175206 is not needed for the push to jdk8. Just a few comments on String.join (which overall, it nice and simple). The javadoc doesn't make it obvious that the elements can include null, are you planning to make that clear? A minor nit in the first example there is there is a space before the closing parentheses. It looks to be a bit odd to have one of the @see tags before the @throws and the other after it. In one method the @return wraps around without indenting, in the other method the first @param is nicely indented but the second one is pushed in by extra space. The examples in the second String.join can probably use diamond. I don't know how this looks in the generated javadoc but might be clearer to put each of the strings.add on their own lines. There is also inconsistencies with spaces and parentheses in these examples. I see you proposing to put the test for String.join in test/java/lang/StringTest.java but that is inconsistent with the current organization. I realize this is TestNG test that is not in the unnamed package but test/java/lang is just too high up in the tree for a specific test. Also "StringTest" is a bit general a name given that it is specific to String.join. The usual copyright header for tests is the pure GPL header. I plan to look at StringJoiner in more detail later. -Alan
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
You are fiddling with the javadoc for getChars, which is an independent change. (I am also fiddling with getChars in another ongoing change). I don't think closing html tags for <li> are required in javadoc. If you are going to change the exception javadoc, then also change @exception to @throws. On Thu, Apr 11, 2013 at 3:33 PM, Jim Gish <jim.gish@oracle.com> wrote:
Please review http://cr.openjdk.java.net/~**jgish/Bugs-5015163-7175206-** 7172553/ <http://cr.openjdk.java.net/~jgish/Bugs-5015163-7175206-7172553/>< http://cr.openjdk.java.net/%**7Ejgish/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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
On 04/15/2013 02:02 PM, Martin Buchholz wrote:
You are fiddling with the javadoc for getChars, which is an independent change. (I am also fiddling with getChars in another ongoing change). I don't think closing html tags for <li> are required in javadoc. If you are going to change the exception javadoc, then also change @exception to @throws.
The only reason I'm adding </li> is Alan insisted on it in a previous change I proposed :-) Jim
On Thu, Apr 11, 2013 at 3:33 PM, Jim Gish <jim.gish@oracle.com <mailto:jim.gish@oracle.com>> wrote:
Please review http://cr.openjdk.java.net/~jgish/Bugs-5015163-7175206-7172553/ <http://cr.openjdk.java.net/%7Ejgish/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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 <tel:%2B1.781.442.0304> Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com <mailto:jim.gish@oracle.com>
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
On 16/04/2013 16:13, Jim Gish wrote:
On 04/15/2013 02:02 PM, Martin Buchholz wrote:
You are fiddling with the javadoc for getChars, which is an independent change. (I am also fiddling with getChars in another ongoing change). I don't think closing html tags for <li> are required in javadoc. If you are going to change the exception javadoc, then also change @exception to @throws.
The only reason I'm adding </li> is Alan insisted on it in a previous change I proposed :-)
I don't recall the full context but I have got confused by an early build of doclint where this was an issue. -Alan.
On Apr 16 2013, at 08:50 , Alan Bateman wrote:
On 16/04/2013 16:13, Jim Gish wrote:
On 04/15/2013 02:02 PM, Martin Buchholz wrote:
You are fiddling with the javadoc for getChars, which is an independent change. (I am also fiddling with getChars in another ongoing change). I don't think closing html tags for <li> are required in javadoc. If you are going to change the exception javadoc, then also change @exception to @throws.
The only reason I'm adding </li> is Alan insisted on it in a previous change I proposed :-)
I don't recall the full context but I have got confused by an early build of doclint where this was an issue.
</li> is required by XHTML but not by HTML. doclint was too aggressive about this in early builds. Having </li> does no harm but it's not required. Mike
-Alan.
I'm going to rip out the </li> then. It's an unnecessary burden. Thanks Jim On 04/16/2013 06:50 PM, Mike Duigou wrote:
On Apr 16 2013, at 08:50 , Alan Bateman wrote:
On 16/04/2013 16:13, Jim Gish wrote:
On 04/15/2013 02:02 PM, Martin Buchholz wrote:
You are fiddling with the javadoc for getChars, which is an independent change. (I am also fiddling with getChars in another ongoing change). I don't think closing html tags for <li> are required in javadoc. If you are going to change the exception javadoc, then also change @exception to @throws.
The only reason I'm adding </li> is Alan insisted on it in a previous change I proposed :-)
I don't recall the full context but I have got confused by an early build of doclint where this was an issue. </li> is required by XHTML but not by HTML. doclint was too aggressive about this in early builds.
Having </li> does no harm but it's not required.
Mike
-Alan.
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
Is "i+1" really preferred to "i + 1" ? I thought it was the reverse, and that "i+1" was merely tolerated. 1570 if (value[i] == hi && value[i+1] == lo) { --- 2425 * @throws NullPointerException If {@code delimiter} is {@code null} you also throw NPE for element null, but you didn't specify that. * * *---* On Thu, Apr 11, 2013 at 3:33 PM, Jim Gish <jim.gish@oracle.com> wrote:
Please review http://cr.openjdk.java.net/~**jgish/Bugs-5015163-7175206-** 7172553/ <http://cr.openjdk.java.net/~jgish/Bugs-5015163-7175206-7172553/>< http://cr.openjdk.java.net/%**7Ejgish/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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
It is natural to compare StringJoiner with guava Joiner. http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/bas... Joiner is popular and has stood the test of time. Joiner designers chose not to include a prefix and suffix, presumably because that is an independent concern - it's not a "joining" activity. OTOH, I'm guessing you are trying to improve the performance of operations like List.toString. More efficient (single copy char[]) would be to collect all the sub-CharSequences in a CharSequence[], pre-compute the final length of the char[], allocate an array of exactly the required length, and create the final string directly from that using the package-private constructor (but in the unlikely event that a subsequence changed in size while concat'ing, be prepared to resize the array). On Thu, Apr 11, 2013 at 3:33 PM, Jim Gish <jim.gish@oracle.com> wrote:
Please review http://cr.openjdk.java.net/~**jgish/Bugs-5015163-7175206-** 7172553/ <http://cr.openjdk.java.net/~jgish/Bugs-5015163-7175206-7172553/>< http://cr.openjdk.java.net/%**7Ejgish/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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
On Mon, Apr 15, 2013 at 11:31 AM, Martin Buchholz <martinrb@google.com>wrote:
OTOH, I'm guessing you are trying to improve the performance of operations like List.toString. More efficient (single copy char[]) would be to collect all the sub-CharSequences in a CharSequence[], pre-compute the final length of the char[], allocate an array of exactly the required length, and create the final string directly from that using the package-private constructor (but in the unlikely event that a subsequence changed in size while concat'ing, be prepared to resize the array).
Proceeding further along this train of thought, I might start with AbstractCollection.toString() (and similar methods) and attempt to make it maximally efficient. Maybe add a method to JavaLangAccess to make a String directly from a perfectly sized array (as needed elsewhere?). Maybe create a StringBuilder-like class that works better for typical use cases? Software is hard.
On Apr 15, 2013, at 12:21 PM, Martin Buchholz <martinrb@google.com> wrote:
On Mon, Apr 15, 2013 at 11:31 AM, Martin Buchholz <martinrb@google.com>wrote:
OTOH, I'm guessing you are trying to improve the performance of operations like List.toString. More efficient (single copy char[]) would be to collect all the sub-CharSequences in a CharSequence[], pre-compute the final length of the char[], allocate an array of exactly the required length, and create the final string directly from that using the package-private constructor (but in the unlikely event that a subsequence changed in size while concat'ing, be prepared to resize the array).
Proceeding further along this train of thought, I might start with AbstractCollection.toString() (and similar methods) and attempt to make it maximally efficient. Maybe add a method to JavaLangAccess to make a String directly from a perfectly sized array (as needed elsewhere?). Maybe create a StringBuilder-like class that works better for typical use cases?
For what it's worth, a patch that I contributed and Mike (and others) then rewrote contains this functionality already: http://cr.openjdk.java.net/~mduigou/JDK-8006627/2/webrev/src/share/classes/s... It's not merged yet though.
Thanks for the pointer. Yeah, that's one the pieces I think we should have to do an optimal job of rewriting collection toString methods. On Mon, Apr 15, 2013 at 1:03 PM, Steven Schlansker < stevenschlansker@gmail.com> wrote:
On Apr 15, 2013, at 12:21 PM, Martin Buchholz <martinrb@google.com> wrote:
On Mon, Apr 15, 2013 at 11:31 AM, Martin Buchholz <martinrb@google.com wrote:
OTOH, I'm guessing you are trying to improve the performance of
operations
like List.toString. More efficient (single copy char[]) would be to collect all the sub-CharSequences in a CharSequence[], pre-compute the final length of the char[], allocate an array of exactly the required length, and create the final string directly from that using the package-private constructor (but in the unlikely event that a subsequence changed in size while concat'ing, be prepared to resize the array).
Proceeding further along this train of thought, I might start with AbstractCollection.toString() (and similar methods) and attempt to make it maximally efficient. Maybe add a method to JavaLangAccess to make a String directly from a perfectly sized array (as needed elsewhere?). Maybe create a StringBuilder-like class that works better for typical use cases?
For what it's worth, a patch that I contributed and Mike (and others) then rewrote contains this functionality already:
http://cr.openjdk.java.net/~mduigou/JDK-8006627/2/webrev/src/share/classes/s...
It's not merged yet though.
On Apr 15 2013, at 13:03 , Steven Schlansker wrote:
On Apr 15, 2013, at 12:21 PM, Martin Buchholz <martinrb@google.com> wrote:
On Mon, Apr 15, 2013 at 11:31 AM, Martin Buchholz <martinrb@google.com>wrote:
OTOH, I'm guessing you are trying to improve the performance of operations like List.toString. More efficient (single copy char[]) would be to collect all the sub-CharSequences in a CharSequence[], pre-compute the final length of the char[], allocate an array of exactly the required length, and create the final string directly from that using the package-private constructor (but in the unlikely event that a subsequence changed in size while concat'ing, be prepared to resize the array).
Proceeding further along this train of thought, I might start with AbstractCollection.toString() (and similar methods) and attempt to make it maximally efficient. Maybe add a method to JavaLangAccess to make a String directly from a perfectly sized array (as needed elsewhere?). Maybe create a StringBuilder-like class that works better for typical use cases?
For what it's worth, a patch that I contributed and Mike (and others) then rewrote contains this functionality already:
http://cr.openjdk.java.net/~mduigou/JDK-8006627/2/webrev/src/share/classes/s...
It's not merged yet though.
It is not forgotten. :-)
String:: line 1253: This should use {@code } rather than <code></code>. I think regular spaces are OK as well. seems inappropriate. lines 2425/2467: elements may not be null either. I can tell you (or maybe it's just me) are itching to change : for (CharSequence cs: elements) { 2477 joiner.add(cs); 2478 } to: elements.forEach(joiner::add); StringJoiner:: - <blockquote> isn't needed around <pre> as it's already a <div> you probably mean to do <pre> {@code ... }</pre> for code samples. - It would be nice if the empty output generation in three arg constructor could be suppressed unless needed. Perhaps a special (not null please!) sentinel value? - Four arg constructor doesn't include emptyOutput in @throws NPE On Apr 11 2013, at 15: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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
Here's an update: http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/ <http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/> Jim On 04/17/2013 03:15 PM, Mike Duigou wrote:
String::
line 1253: This should use {@code } rather than <code></code>. I think regular spaces are OK as well. seems inappropriate.
lines 2425/2467: elements may not be null either.
I can tell you (or maybe it's just me) are itching to change :
for (CharSequence cs: elements) { 2477 joiner.add(cs); 2478 }
to:
elements.forEach(joiner::add);
StringJoiner::
- <blockquote> isn't needed around <pre> as it's already a <div> you probably mean to do
<pre> {@code ... }</pre>
for code samples.
- It would be nice if the empty output generation in three arg constructor could be suppressed unless needed. Perhaps a special (not null please!) sentinel value?
- Four arg constructor doesn't include emptyOutput in @throws NPE
On Apr 11 2013, at 15: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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
I'm still wondering about whether a joiner utility should support a prefix and suffix. The obvious uses for this are collection class toString methods, but we already know that we can and should implement those with a single precise char[] construction, so should not use StringJoiner, or at least not this StringJoiner implementation. And if we're just talking about pure convenience, it's hard to beat "[" + String.join(...) + "]" On Wed, Apr 17, 2013 at 2:49 PM, Jim Gish <jim.gish@oracle.com> wrote:
Here's an update: http://cr.openjdk.java.net/~** jgish/Bugs-5015163-7172553/<http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/>< http://cr.openjdk.java.net/%**7Ejgish/Bugs-5015163-7172553/<http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/>
Jim
On 04/17/2013 03:15 PM, Mike Duigou wrote:
String::
line 1253: This should use {@code } rather than <code></code>. I think regular spaces are OK as well. seems inappropriate.
lines 2425/2467: elements may not be null either.
I can tell you (or maybe it's just me) are itching to change :
for (CharSequence cs: elements) { 2477 joiner.add(cs); 2478 }
to:
elements.forEach(joiner::add);
StringJoiner::
- <blockquote> isn't needed around <pre> as it's already a <div> you probably mean to do
<pre> {@code ... }</pre>
for code samples.
- It would be nice if the empty output generation in three arg constructor could be suppressed unless needed. Perhaps a special (not null please!) sentinel value?
- Four arg constructor doesn't include emptyOutput in @throws NPE
On Apr 11 2013, at 15:33 , Jim Gish wrote:
Please review http://cr.openjdk.java.net/~**jgish/Bugs-5015163-7175206-*
*7172553/<http://cr.openjdk.java.net/~jgish/Bugs-5015163-7175206-7172553/>< http://cr.openjdk.java.net/%**7Ejgish/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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
I'm open to this, but am interested in what others have to say, especially as it relates to other lambda features coming in. Bear in mind that this is at least the third major round of reviews for these changes, the first round being a year ago on lambda-dev, when I first submitted them, and then they were distilled some more and greatly simplified by Henry Jen. Thanks, Jim On 04/17/2013 06:07 PM, Martin Buchholz wrote:
I'm still wondering about whether a joiner utility should support a prefix and suffix. The obvious uses for this are collection class toString methods, but we already know that we can and should implement those with a single precise char[] construction, so should not use StringJoiner, or at least not this StringJoiner implementation. And if we're just talking about pure convenience, it's hard to beat
"[" + String.join(...) + "]"
On Wed, Apr 17, 2013 at 2:49 PM, Jim Gish <jim.gish@oracle.com <mailto:jim.gish@oracle.com>> wrote:
Here's an update: http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/ <http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/> <http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/>
Jim
On 04/17/2013 03:15 PM, Mike Duigou wrote:
String::
line 1253: This should use {@code } rather than <code></code>. I think regular spaces are OK as well. seems inappropriate.
lines 2425/2467: elements may not be null either.
I can tell you (or maybe it's just me) are itching to change :
for (CharSequence cs: elements) { 2477 joiner.add(cs); 2478 }
to:
elements.forEach(joiner::add);
StringJoiner::
- <blockquote> isn't needed around <pre> as it's already a <div> you probably mean to do
<pre> {@code ... }</pre>
for code samples.
- It would be nice if the empty output generation in three arg constructor could be suppressed unless needed. Perhaps a special (not null please!) sentinel value?
- Four arg constructor doesn't include emptyOutput in @throws NPE
On Apr 11 2013, at 15: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/> <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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 <tel:%2B1.781.442.0304> Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com <mailto:jim.gish@oracle.com>
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 <tel:%2B1.781.442.0304> Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com <mailto:jim.gish@oracle.com>
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
The motivation was indeed that it would support more efficient Collection.toString. (But, I don't believe anything actually uses that feature right now, other than tests.) Even if *our* implementations were not to use this because we had a better for-experts construction, I still think this is a useful feature that users classes may benefit from in their own toString methods. On 4/17/2013 6:15 PM, Jim Gish wrote:
I'm open to this, but am interested in what others have to say, especially as it relates to other lambda features coming in. Bear in mind that this is at least the third major round of reviews for these changes, the first round being a year ago on lambda-dev, when I first submitted them, and then they were distilled some more and greatly simplified by Henry Jen.
Thanks, Jim
On 04/17/2013 06:07 PM, Martin Buchholz wrote:
I'm still wondering about whether a joiner utility should support a prefix and suffix. The obvious uses for this are collection class toString methods, but we already know that we can and should implement those with a single precise char[] construction, so should not use StringJoiner, or at least not this StringJoiner implementation. And if we're just talking about pure convenience, it's hard to beat
"[" + String.join(...) + "]"
On Wed, Apr 17, 2013 at 2:49 PM, Jim Gish <jim.gish@oracle.com <mailto:jim.gish@oracle.com>> wrote:
Here's an update: http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/ <http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/> <http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/>
Jim
On 04/17/2013 03:15 PM, Mike Duigou wrote:
String::
line 1253: This should use {@code } rather than <code></code>. I think regular spaces are OK as well. seems inappropriate.
lines 2425/2467: elements may not be null either.
I can tell you (or maybe it's just me) are itching to change :
for (CharSequence cs: elements) { 2477 joiner.add(cs); 2478 }
to:
elements.forEach(joiner::add);
StringJoiner::
- <blockquote> isn't needed around <pre> as it's already a <div> you probably mean to do
<pre> {@code ... }</pre>
for code samples.
- It would be nice if the empty output generation in three arg constructor could be suppressed unless needed. Perhaps a special (not null please!) sentinel value?
- Four arg constructor doesn't include emptyOutput in @throws NPE
On Apr 11 2013, at 15: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/>
<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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 <tel:%2B1.781.442.0304> Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com <mailto:jim.gish@oracle.com>
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 <tel:%2B1.781.442.0304> Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com <mailto:jim.gish@oracle.com>
To be concrete, here's a proposed toString method : http://cr.openjdk.java.net/~martin/webrevs/openjdk8/toString/ which is pretty good as it stands, but even better once it gets to use the no-copy String constructor.
If I don't get a chance to revisit the whole UUID optimization patch soon I will see what I can do about breaking it up further and maybe just do the no-copy String constructor by itself. Mike On Apr 17 2013, at 20:29 , Martin Buchholz wrote:
To be concrete, here's a proposed toString method : http://cr.openjdk.java.net/~martin/webrevs/openjdk8/toString/ which is pretty good as it stands, but even better once it gets to use the no-copy String constructor.
Hi, I'm wondering, that StringJoiner has some logic for pre/suffix, but nothing to loop the elements themselves :-( To me, StringJoiner is a useless complicated box around StringBuilder, and imagine, someone needs thread-safety. It also slows down performance, as it needs additional instances and additional class to be loaded (critical at VM startup). Instead please add to StringBuilder and StringBuffer: append(CharSequence... elements); append(char delimiter, CharSequence... elements); append(char delimiter, Iterable<? extends CharSequence> elements); cut(int len); // removes len chars at the end of the sequence optional: append(CharSequence delimiter, CharSequence... elements); append(CharSequence delimiter, Iterable<? extends CharSequence> elements); For performance reasons, append should always append the trailing delimeter, which could be cut at the end. It's questionable, if class string needs a static (=no relation to an existing string in contrast to non-static split()) join method, as it seduces to "[" + String.join(...) + "]" which needs some effort from javac side to optimize to a single StringBuilder task. IMO we better had StringBuilder.join(...), so javac could easily optimize to: new StringBuilder().append('[').append(',', someStrings).cut(1).append(']').toString() -Ulf Am 18.04.2013 00:07, schrieb Martin Buchholz:
I'm still wondering about whether a joiner utility should support a prefix and suffix. The obvious uses for this are collection class toString methods, but we already know that we can and should implement those with a single precise char[] construction, so should not use StringJoiner, or at least not this StringJoiner implementation. And if we're just talking about pure convenience, it's hard to beat
"[" + String.join(...) + "]"
On Wed, Apr 17, 2013 at 2:49 PM, Jim Gish <jim.gish@oracle.com> wrote:
Here's an update: http://cr.openjdk.java.net/~** jgish/Bugs-5015163-7172553/<http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/>< http://cr.openjdk.java.net/%**7Ejgish/Bugs-5015163-7172553/<http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/> Jim
On 04/17/2013 03:15 PM, Mike Duigou wrote:
String::
line 1253: This should use {@code } rather than <code></code>. I think regular spaces are OK as well. seems inappropriate.
lines 2425/2467: elements may not be null either.
I can tell you (or maybe it's just me) are itching to change :
for (CharSequence cs: elements) { 2477 joiner.add(cs); 2478 }
to:
elements.forEach(joiner::add);
StringJoiner::
- <blockquote> isn't needed around <pre> as it's already a <div> you probably mean to do
<pre> {@code ... }</pre>
for code samples.
- It would be nice if the empty output generation in three arg constructor could be suppressed unless needed. Perhaps a special (not null please!) sentinel value?
- Four arg constructor doesn't include emptyOutput in @throws NPE
On Apr 11 2013, at 15:33 , Jim Gish wrote:
Please review http://cr.openjdk.java.net/~**jgish/Bugs-5015163-7175206-*
*7172553/<http://cr.openjdk.java.net/~jgish/Bugs-5015163-7175206-7172553/>< http://cr.openjdk.java.net/%**7Ejgish/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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
On 04/18/2013 08:49 AM, Ulf Zibis wrote:
Hi,
I'm wondering, that StringJoiner has some logic for pre/suffix, but nothing to loop the elements themselves :-(
To me, StringJoiner is a useless complicated box around StringBuilder, and imagine, someone needs thread-safety. It also slows down performance, as it needs additional instances and additional class to be loaded (critical at VM startup).
Instead please add to StringBuilder and StringBuffer: append(CharSequence... elements); append(char delimiter, CharSequence... elements); append(char delimiter, Iterable<? extends CharSequence> elements); cut(int len); // removes len chars at the end of the sequence optional: append(CharSequence delimiter, CharSequence... elements); append(CharSequence delimiter, Iterable<? extends CharSequence> elements); I started off with something similar, but it was stripped out when Henry did the performance improvements. Given that most people feel that this is going to be put to heavy-weight usage, it doesn't seem to merit too much emphasis on performance or complicating the implementation at this point.
Thanks
For performance reasons, append should always append the trailing delimeter, which could be cut at the end.
It's questionable, if class string needs a static (=no relation to an existing string in contrast to non-static split()) join method, as it seduces to "[" + String.join(...) + "]" which needs some effort from javac side to optimize to a single StringBuilder task. IMO we better had StringBuilder.join(...), so javac could easily optimize to: new StringBuilder().append('[').append(',', someStrings).cut(1).append(']').toString()
-Ulf
Am 18.04.2013 00:07, schrieb Martin Buchholz:
I'm still wondering about whether a joiner utility should support a prefix and suffix. The obvious uses for this are collection class toString methods, but we already know that we can and should implement those with a single precise char[] construction, so should not use StringJoiner, or at least not this StringJoiner implementation. And if we're just talking about pure convenience, it's hard to beat
"[" + String.join(...) + "]"
On Wed, Apr 17, 2013 at 2:49 PM, Jim Gish <jim.gish@oracle.com> wrote:
Here's an update: http://cr.openjdk.java.net/~** jgish/Bugs-5015163-7172553/<http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/><
Jim
On 04/17/2013 03:15 PM, Mike Duigou wrote:
String::
line 1253: This should use {@code } rather than <code></code>. I think regular spaces are OK as well. seems inappropriate.
lines 2425/2467: elements may not be null either.
I can tell you (or maybe it's just me) are itching to change :
for (CharSequence cs: elements) { 2477 joiner.add(cs); 2478 }
to:
elements.forEach(joiner::add);
StringJoiner::
- <blockquote> isn't needed around <pre> as it's already a <div> you probably mean to do
<pre> {@code ... }</pre>
for code samples.
- It would be nice if the empty output generation in three arg constructor could be suppressed unless needed. Perhaps a special (not null please!) sentinel value?
- Four arg constructor doesn't include emptyOutput in @throws NPE
On Apr 11 2013, at 15:33 , Jim Gish wrote:
Please review http://cr.openjdk.java.net/~**jgish/Bugs-5015163-7175206-*
*7172553/<http://cr.openjdk.java.net/~jgish/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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
Garbled javadoc: 41 * method will, by default, return {@code prefix+{@code suffix}}. However, if
Thanks for catching that. I thought I fixed it. (In fact, I'm sure I did in the latest rev). On 04/18/2013 05:03 PM, Martin Buchholz wrote:
Garbled javadoc:
41 * method will, by default, return {@code prefix+{@code suffix}}. However, if
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
Am 18.04.2013 19:37, schrieb Jim Gish:
On 04/18/2013 08:49 AM, Ulf Zibis wrote:
Hi,
I'm wondering, that StringJoiner has some logic for pre/suffix, but nothing to loop the elements themselves :-(
To me, StringJoiner is a useless complicated box around StringBuilder, and imagine, someone needs thread-safety. It also slows down performance, as it needs additional instances and additional class to be loaded (critical at VM startup).
Instead please add to StringBuilder and StringBuffer: append(CharSequence... elements); append(char delimiter, CharSequence... elements); append(char delimiter, Iterable<? extends CharSequence> elements); cut(int len); // removes len chars at the end of the sequence optional: append(CharSequence delimiter, CharSequence... elements); append(CharSequence delimiter, Iterable<? extends CharSequence> elements); I started off with something similar, but it was stripped out when Henry did the performance improvements.
Hm, I have no idea, how above suggestions should prevent performance improvements. Can you help me?
Given that most people feel that this is going to be put to heavy-weight usage, it doesn't seem to merit too much emphasis on performance or complicating the implementation at this point.
Your implementation has 1 class with 7 methods 2 additional methods in String To cover the same functionality, above approach basically only needs 2 additional methods in StringBuilder, has better performance, so what is complicated on that? @Martin: What is your opinion? Thanks, -Ulf
For performance reasons, append should always append the trailing delimeter, which could be cut at the end.
It's questionable, if class string needs a static (=no relation to an existing string in contrast to non-static split()) join method, as it seduces to "[" + String.join(...) + "]" which needs some effort from javac side to optimize to a single StringBuilder task. IMO we better had StringBuilder.join(...), so javac could easily optimize to: new StringBuilder().append('[').append(',', someStrings).cut(1).append(']').toString()
-Ulf
Am 18.04.2013 00:07, schrieb Martin Buchholz:
I'm still wondering about whether a joiner utility should support a prefix and suffix. The obvious uses for this are collection class toString methods, but we already know that we can and should implement those with a single precise char[] construction, so should not use StringJoiner, or at least not this StringJoiner implementation. And if we're just talking about pure convenience, it's hard to beat
"[" + String.join(...) + "]"
On Wed, Apr 17, 2013 at 2:49 PM, Jim Gish <jim.gish@oracle.com> wrote:
Here's an update: http://cr.openjdk.java.net/~** jgish/Bugs-5015163-7172553/<http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/>< http://cr.openjdk.java.net/%**7Ejgish/Bugs-5015163-7172553/<http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/>
Jim
Hi Henry, Can you please comment on the simplifications you did ? Thanks, Jim On 04/18/2013 07:38 PM, Ulf Zibis wrote:
Am 18.04.2013 19:37, schrieb Jim Gish:
On 04/18/2013 08:49 AM, Ulf Zibis wrote:
Hi,
I'm wondering, that StringJoiner has some logic for pre/suffix, but nothing to loop the elements themselves :-(
To me, StringJoiner is a useless complicated box around StringBuilder, and imagine, someone needs thread-safety. It also slows down performance, as it needs additional instances and additional class to be loaded (critical at VM startup).
Instead please add to StringBuilder and StringBuffer: append(CharSequence... elements); append(char delimiter, CharSequence... elements); append(char delimiter, Iterable<? extends CharSequence> elements); cut(int len); // removes len chars at the end of the sequence optional: append(CharSequence delimiter, CharSequence... elements); append(CharSequence delimiter, Iterable<? extends CharSequence> elements); I started off with something similar, but it was stripped out when Henry did the performance improvements.
Hm, I have no idea, how above suggestions should prevent performance improvements. Can you help me?
Given that most people feel that this is going to be put to heavy-weight usage, it doesn't seem to merit too much emphasis on performance or complicating the implementation at this point.
Your implementation has 1 class with 7 methods 2 additional methods in String To cover the same functionality, above approach basically only needs 2 additional methods in StringBuilder, has better performance, so what is complicated on that?
@Martin: What is your opinion?
Thanks,
-Ulf
For performance reasons, append should always append the trailing delimeter, which could be cut at the end.
It's questionable, if class string needs a static (=no relation to an existing string in contrast to non-static split()) join method, as it seduces to "[" + String.join(...) + "]" which needs some effort from javac side to optimize to a single StringBuilder task. IMO we better had StringBuilder.join(...), so javac could easily optimize to: new StringBuilder().append('[').append(',', someStrings).cut(1).append(']').toString()
-Ulf
Am 18.04.2013 00:07, schrieb Martin Buchholz:
I'm still wondering about whether a joiner utility should support a prefix and suffix. The obvious uses for this are collection class toString methods, but we already know that we can and should implement those with a single precise char[] construction, so should not use StringJoiner, or at least not this StringJoiner implementation. And if we're just talking about pure convenience, it's hard to beat
"[" + String.join(...) + "]"
On Wed, Apr 17, 2013 at 2:49 PM, Jim Gish <jim.gish@oracle.com> wrote:
Here's an update: http://cr.openjdk.java.net/~** jgish/Bugs-5015163-7172553/<http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/><
Jim
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
Sorry I am not receiving emails from core-lib-dev earlier. What I did was to remove things we considered not absolutely required, which left us only a StringJoiner class and two String.join methods. StringJoiner was intended to be used with Stream as a collector, thus prefix, and suffix is desired like Collection.toString(). Cheers, Henry On Apr 19, 2013, at 9:54 AM, Jim Gish <jim.gish@oracle.com> wrote:
Hi Henry, Can you please comment on the simplifications you did ?
Thanks, Jim
On 04/18/2013 07:38 PM, Ulf Zibis wrote:
Am 18.04.2013 19:37, schrieb Jim Gish:
On 04/18/2013 08:49 AM, Ulf Zibis wrote:
Hi,
I'm wondering, that StringJoiner has some logic for pre/suffix, but nothing to loop the elements themselves :-(
To me, StringJoiner is a useless complicated box around StringBuilder, and imagine, someone needs thread-safety. It also slows down performance, as it needs additional instances and additional class to be loaded (critical at VM startup).
Instead please add to StringBuilder and StringBuffer: append(CharSequence... elements); append(char delimiter, CharSequence... elements); append(char delimiter, Iterable<? extends CharSequence> elements); cut(int len); // removes len chars at the end of the sequence optional: append(CharSequence delimiter, CharSequence... elements); append(CharSequence delimiter, Iterable<? extends CharSequence> elements); I started off with something similar, but it was stripped out when Henry did the performance improvements.
Hm, I have no idea, how above suggestions should prevent performance improvements. Can you help me?
Given that most people feel that this is going to be put to heavy-weight usage, it doesn't seem to merit too much emphasis on performance or complicating the implementation at this point.
Your implementation has 1 class with 7 methods 2 additional methods in String To cover the same functionality, above approach basically only needs 2 additional methods in StringBuilder, has better performance, so what is complicated on that?
@Martin: What is your opinion?
Thanks,
-Ulf
For performance reasons, append should always append the trailing delimeter, which could be cut at the end.
It's questionable, if class string needs a static (=no relation to an existing string in contrast to non-static split()) join method, as it seduces to "[" + String.join(...) + "]" which needs some effort from javac side to optimize to a single StringBuilder task. IMO we better had StringBuilder.join(...), so javac could easily optimize to: new StringBuilder().append('[').append(',', someStrings).cut(1).append(']').toString()
-Ulf
Am 18.04.2013 00:07, schrieb Martin Buchholz:
I'm still wondering about whether a joiner utility should support a prefix and suffix. The obvious uses for this are collection class toString methods, but we already know that we can and should implement those with a single precise char[] construction, so should not use StringJoiner, or at least not this StringJoiner implementation. And if we're just talking about pure convenience, it's hard to beat
"[" + String.join(...) + "]"
On Wed, Apr 17, 2013 at 2:49 PM, Jim Gish <jim.gish@oracle.com> wrote:
Here's an update: http://cr.openjdk.java.net/~** jgish/Bugs-5015163-7172553/<http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/>< http://cr.openjdk.java.net/%**7Ejgish/Bugs-5015163-7172553/<http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/> Jim
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
Hi, Can I suggest that the StringJoiner.toString() method explicitly append the suffix to the existing StringBuilder. 152 return (value != null ? value.toString() + suffix : emptyValue); Currently it will go to the trouble of creating a String from the builder and then transparently create another StringBuilder to do the concatenation. 152 return (value != null ? value.append(suffix).toString() : emptyValue); or something similar. The overhead of StringJoiner supporting prefix and suffix is lower than doing it separately in terms of object allocations and garbage and all places that would need write their own code to do the concatenation. Roger On 4/17/13 5:49 PM, 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
On 04/17/2013 03:15 PM, Mike Duigou wrote:
String::
line 1253: This should use {@code } rather than <code></code>. I think regular spaces are OK as well. seems inappropriate.
lines 2425/2467: elements may not be null either.
I can tell you (or maybe it's just me) are itching to change :
for (CharSequence cs: elements) { 2477 joiner.add(cs); 2478 }
to:
elements.forEach(joiner::add);
StringJoiner::
- <blockquote> isn't needed around <pre> as it's already a <div> you probably mean to do
<pre> {@code ... }</pre>
for code samples.
- It would be nice if the empty output generation in three arg constructor could be suppressed unless needed. Perhaps a special (not null please!) sentinel value?
- Four arg constructor doesn't include emptyOutput in @throws NPE
On Apr 11 2013, at 15: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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
Hi, I don't think it's worth to add the braces at lines 2359..2360. Please swap lines 2740 <-> 2739. -Ulf Am 18.04.2013 03:20, schrieb Roger Riggs:
Hi,
Can I suggest that the StringJoiner.toString() method explicitly append the suffix to the existing StringBuilder.
152 return (value != null ? value.toString() + suffix : emptyValue);
Currently it will go to the trouble of creating a String from the builder and then transparently create another StringBuilder to do the concatenation.
152 return (value != null ? value.append(suffix).toString() : emptyValue);
or something similar.
The overhead of StringJoiner supporting prefix and suffix is lower than doing it separately in terms of object allocations and garbage and all places that would need write their own code to do the concatenation.
Roger
On 4/17/13 5:49 PM, 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
That was a nice idea, but you don't want to change the value when you do toString(). Otherwise, if you subsequently add a new element, you're hosed because you've already added on the suffix. Jim On 04/17/2013 09:20 PM, Roger Riggs wrote:
Hi,
Can I suggest that the StringJoiner.toString() method explicitly append the suffix to the existing StringBuilder.
152 return (value != null ? value.toString() + suffix : emptyValue);
Currently it will go to the trouble of creating a String from the builder and then transparently create another StringBuilder to do the concatenation.
152 return (value != null ? value.append(suffix).toString() : emptyValue);
or something similar.
The overhead of StringJoiner supporting prefix and suffix is lower than doing it separately in terms of object allocations and garbage and all places that would need write their own code to do the concatenation.
Roger
On 4/17/13 5:49 PM, 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
On 04/17/2013 03:15 PM, Mike Duigou wrote:
String::
line 1253: This should use {@code } rather than <code></code>. I think regular spaces are OK as well. seems inappropriate.
lines 2425/2467: elements may not be null either.
I can tell you (or maybe it's just me) are itching to change :
for (CharSequence cs: elements) { 2477 joiner.add(cs); 2478 }
to:
elements.forEach(joiner::add);
StringJoiner::
- <blockquote> isn't needed around <pre> as it's already a <div> you probably mean to do
<pre> {@code ... }</pre>
for code samples.
- It would be nice if the empty output generation in three arg constructor could be suppressed unless needed. Perhaps a special (not null please!) sentinel value?
- Four arg constructor doesn't include emptyOutput in @throws NPE
On Apr 11 2013, at 15: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
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
On Thu, Apr 18, 2013 at 10:34 AM, Jim Gish <jim.gish@oracle.com> wrote:
That was a nice idea, but you don't want to change the value when you do toString(). Otherwise, if you subsequently add a new element, you're hosed because you've already added on the suffix.
You can cheaply save the current length, append the suffix, call toString, and reset the length back to the old value to avoid the overhead of String "+".
Martin, I've updated the toString() method as you suggested. http://cr.openjdk.java.net/~jgish/Bugs-5015163-7172553/ <http://cr.openjdk.java.net/%7Ejgish/Bugs-5015163-7172553/> Thanks, Jim On 04/18/2013 05:02 PM, Martin Buchholz wrote:
On Thu, Apr 18, 2013 at 10:34 AM, Jim Gish <jim.gish@oracle.com <mailto:jim.gish@oracle.com>> wrote:
That was a nice idea, but you don't want to change the value when you do toString(). Otherwise, if you subsequently add a new element, you're hosed because you've already added on the suffix.
You can cheaply save the current length, append the suffix, call toString, and reset the length back to the old value to avoid the overhead of String "+".
-- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.gish@oracle.com
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.
didn't see delimiter collision mentioned anywhere - are you really offering an interface that only joins a list of entries without escaping or quoting or even checking for separators (or escape or quote sequences) that might occur in the entries? -- View this message in context: http://openjdk.5641.n7.nabble.com/RFR-String-join-StringJoiner-additions-tp1... Sent from the OpenJDK Core Libraries mailing list archive at Nabble.com.
On Jul 23 2013, at 12:43 , ph wrote:
didn't see delimiter collision mentioned anywhere - are you really offering an interface that only joins a list of entries without escaping or quoting or even checking for separators (or escape or quote sequences) that might occur in the entries?
Correct. StringJoiner makes no effort to address escaping/quoting. Doing so would add a lot of complexity that would not useful interesting to most users and probably still wouldn't satisfy everyone. If you wish some form of escaping or quoting you can pre-processed entries however you like before joining them. Mike
The omission of that very complexity is a copious source of bugs that breaks applications in production, and is extremely useful and interesting to the black hat community. It should not be possible to call a joiner without escaping or quoting or catching an exception or explicitly waiving the complexity. Of course there will still be some users who roll their own, blissfully unaware of the existence of StringJoiner. But for those who do discover it, the important value to add is not convenience, but safety. Maybe you could even retrofit it to Arrays.toString(), which is exactly such a toy interface only good for things like logging where delimiter collision doesn't usually matter. At the very least can you please cover it in the examples so that people can't completely overlook it? -- View this message in context: http://openjdk.5641.n7.nabble.com/RFR-String-join-StringJoiner-additions-tp1... Sent from the OpenJDK Core Libraries mailing list archive at Nabble.com.
On Jul 23, 2013, at 1:37 PM, ph <philip.hodges@bluewin.ch> wrote:
The omission of that very complexity is a copious source of bugs that breaks applications in production, and is extremely useful and interesting to the black hat community. It should not be possible to call a joiner without escaping or quoting or catching an exception or explicitly waiving the complexity. Of course there will still be some users who roll their own, blissfully unaware of the existence of StringJoiner. But for those who do discover it, the important value to add is not convenience, but safety. Maybe you could even retrofit it to Arrays.toString(), which is exactly such a toy interface only good for things like logging where delimiter collision doesn't usually matter. At the very least can you please cover it in the examples so that people can't completely overlook it?
Hi, If you are expecting to read the data back in a structured manner, I would expect you to use a proper structured format e.g. CSV or JSON, not a simple string joiner. String joining seems to work the same in every language I see (Python, Ruby, Javascript at least) and given that it takes arbitrary (potentially multi-character) delimiters I'm not even sure what behavior you'd expect that could make sense with any given delimiter. The current behavior is similar to other languages, simple to understand, and seems to not be "surprising" to most people. Why would you not write your data out as e.g. JSON? Then all of the tricky cases are handled correctly for you. Best, Steven
Nothing against a "simple" joiner that forces users to catch or rule out a DelimiterCollisionException, and realise that simple is not good enough. Everything against a stupid joiner that silently assists users in creating bugs. "Seems to work" means not tested thoroughly. People will be tempted into using this joiner to write csv files, and of course it will break as soon as a comma or semicolon delimiter turns up in a text field. They will use it to put br line break elements into HTML files, without checking for HTML syntax characters in each line. They will use it to separate SQL column values with commas, instead of binding them, or calling mysql_real_escape_string, which is a pain to use because it needs a database connection at runtime to find out exactly which characters need escaping depending on which SQL mode is configured. By all means use a "proper structured format", so long as the delimiting is properly specified and the parser and generator are fully tested. That might well be the case for JSON and XML, but it typically is not for CSV and HTML or properties files. Just because other languages may happen to have something similar does not automatically make it easy to use safely. The current behaviour encourages SQL injection. How hard is that to understand? There are some very good features coming in this release, but this joiner is not one of them; it is actually dangerous. -- View this message in context: http://openjdk.5641.n7.nabble.com/RFR-String-join-StringJoiner-additions-tp1... Sent from the OpenJDK Core Libraries mailing list archive at Nabble.com.
On 07/23/2013 11:57 PM, ph wrote:
Nothing against a "simple" joiner that forces users to catch or rule out a DelimiterCollisionException, and realise that simple is not good enough. Everything against a stupid joiner that silently assists users in creating bugs.
"Seems to work" means not tested thoroughly. People will be tempted into using this joiner to write csv files, and of course it will break as soon as a comma or semicolon delimiter turns up in a text field. They will use it to put br line break elements into HTML files, without checking for HTML syntax characters in each line. They will use it to separate SQL column values with commas, instead of binding them, or calling mysql_real_escape_string, which is a pain to use because it needs a database connection at runtime to find out exactly which characters need escaping depending on which SQL mode is configured.
By all means use a "proper structured format", so long as the delimiting is properly specified and the parser and generator are fully tested. That might well be the case for JSON and XML, but it typically is not for CSV and HTML or properties files.
Just because other languages may happen to have something similar does not automatically make it easy to use safely.
The current behaviour encourages SQL injection. How hard is that to understand? There are some very good features coming in this release, but this joiner is not one of them; it is actually dangerous.
Java is not PHP, data are represented by objects not Strings so escaping (if needed*) is done by the API not by the users. Rémi * sql drivers in Java often use binary protocols not textual one.
Are we really going ahead with an implementation that: - checks even string literal parameters for null - does it again when String.join calls StringJoiner - makes defensive copies of just some of the arguments - creates a StringBuilder with only the default capacity People would be better off taking a look at AbstractCollection.toString or Arrays.toString and then, if they haven't already, roll their own, enhanced with a StringBuilder capacity, and without all the defensive baggage. To come back to my original point about why it is a toy interface: The only worthwhile value that StringJoiner can possibly add is a way to force programmers to make a conscious decision to waive checking to see if the separator (or prefix or suffix) can or does occur in any of the elements, and to apply an escaper/quoter if needed. Please add some failing round trip unit tests with split to help raise awareness of the issue of delimiter collision. The original String elements cannot be extracted from the result if there are no elements, one empty element, any null elements, or if the separator occurs in any of the elements. Please add a test of String.join with delimiter and no varargs elements, and another test with one null varargs element. I don't feel comfortable with a signature that does not clearly separate the delimiter from the elements. Please put an example with escaping in the javadoc. You might want to change that ridiculous "Java-is-cool" example. Of course there are a lot of cool things about Java, and even String.join cannot compete with acknowledged uncool features such as Calendar, mutable Dates and unthreadsafe Formatters. Nevertheless, the example text risks becoming ironic. Please take a proper critical look at whether there is any genuine need to dump this half-baked toy into everyone's jdk. There are some good ideas in Java 8. This is not one of them. It will soon be starring behind the scenes in CERT advisories. Once people start using it, even if you do accept and try to fix the flaws, you will then have two versions in circulation for many years. And then I will be able to say "I told you so", instead of "thank goodness we woke up and stopped it so we could get it right first time". Why is there such a bandwagon rolling for this "convenience" feature? It is not "long overdue". Arrays.toString and AbstractCollection.toString have been there for years. We already have joiners in apache commons and guava. At first I thought they were cool. Then I tried to use them in anger. And was forced to roll my own. That can't be right. A more elaborate offically blessed feature that only does half the job is worse than useless. Without the extra complex ability to detect or avoid collisions it is neither "nice", nor a "Good Thing". Phil http://www.techempower.com/blog/2013/03/26/everything-about-java-8/ "StringJoiner and String.join(...) are long, long overdue. They are so long overdue that the vast majority of Java developers likely have already written or have found utilities for joining strings, but it is nice for the JDK to finally provide this itself. Everyone has encountered situations where joining strings is required, and it is a Good Thing™ that we can now express that through a standard API that every Java developer (eventually) will know. " On 2013-07-23 22:09, Mike Duigou wrote:
On Jul 23 2013, at 12:43 , ph wrote:
didn't see delimiter collision mentioned anywhere - are you really offering an interface that only joins a list of entries without escaping or quoting or even checking for separators (or escape or quote sequences) that might occur in the entries?
Correct. StringJoiner makes no effort to address escaping/quoting. Doing so would add a lot of complexity that would not useful interesting to most users and probably still wouldn't satisfy everyone.
If you wish some form of escaping or quoting you can pre-processed entries however you like before joining them.
Mike
On Aug 30 2013, at 23:40 , Philip Hodges wrote:
...
Why is there such a bandwagon rolling for this "convenience" feature?
Perhaps others just don't agree with you. The choice of functionality offered in the JDK 8 StringJoiner was not arbitrary or haphazardly chosen. If it doesn't meet your particular needs, I am sorry to hear that. Our belief as the proposers is that it offers a reasonable mix of functionality and simplicity to cover useful number of requirements. Our intent was certainly not to create the kitchen magician [1] of string joiners that attempted to incorporate every conceivable option. In particular your concerns about escaping are out of scope for the joiner in part because Collection<String> strings; String concat = strings.stream().map(MyUtils::escape).collect(Collections.joining(",")); seems like an adequate and flexible solution to most people.
We already have joiners in apache commons and guava.
This is the reason that StringJoiner was considered for JDK--that many others were "rolling their own".
At first I thought they were cool. Then I tried to use them in anger. And was forced to roll my own.
I would encourage you to share your implementation or the javadocs as grist for discussion. An actual alternative is the *only* thing that is likely to be persuasive.
That can't be right.
Sometimes it is. The JDK doesn't make any attempt to satisfy everyone (or anyone). We all end up writing lots of non-business logic utility functions to bridge gaps in what JDK offers. This is normal. And, if it turns out to be something lots of people need then it's entirely possible that it could be added to the JDK. Mike [1] https://www.youtube.com/watch?v=cGVG9xLHb84
A more elaborate offically blessed feature that only does half the job is worse than useless. Without the extra complex ability to detect or avoid collisions it is neither "nice", nor a "Good Thing".
Phil
http://www.techempower.com/blog/2013/03/26/everything-about-java-8/
"StringJoiner and String.join(...) are long, long overdue. They are so long overdue that the vast majority of Java developers likely have already written or have found utilities for joining strings, but it is nice for the JDK to finally provide this itself. Everyone has encountered situations where joining strings is required, and it is a Good Thing™ that we can now express that through a standard API that every Java developer (eventually) will know. "
On 2013-07-23 22:09, Mike Duigou wrote:
On Jul 23 2013, at 12:43 , ph wrote:
didn't see delimiter collision mentioned anywhere - are you really offering an interface that only joins a list of entries without escaping or quoting or even checking for separators (or escape or quote sequences) that might occur in the entries?
Correct. StringJoiner makes no effort to address escaping/quoting. Doing so would add a lot of complexity that would not useful interesting to most users and probably still wouldn't satisfy everyone.
If you wish some form of escaping or quoting you can pre-processed entries however you like before joining them.
Mike
I don't think my needs are in any way particular or unusual. This is the third attempt at a StringJoiner that has not stopped me having to roll my own. Arrays.toString and AbstractCollection.toString already meet logging needs and serve as simple useful examples for where people don't care about delimiter collision. Your strings/stream/map/escape/collect/joining idiom is impressive and might well be adequate and flexible. It is also quite beyond incomprehensible to many. Even I don't immediately see what the stream and collect methods contribute to the processing, I will look it up later. A lot of those will simply not think about delimiter collision until it bites them in production, because your joiner does not by default insist on either a waiver or an escaper or a checker for separators in the collection where you wrote map(escape). Maybe they will join some numbers first, then copy that code to join some identifiers, and then copy that code to join some names, and it will "work fine". Until one of the names contains an apostrophe or comma. Or they will quickly join a few lines for a web page, separated by a <br> tag, and forget to convert the lines to HTML, or verify that each line is a valid and safe HTML subset fragment. This feature as it stands adds precious little convenience value, while making it easier for people to forget to provide any escaping at all. You are missing a golden opportunity to ensure that the code only compiles if it is complete with a waiver, detector or escaper. I've seen the consequences of escaping being left out all too often. My own rolled code consists of a looper, an appender, and a replacer. I found it best if the appender method that chooses and injects the separator also inserts the quotes and chooses and calls the escaper (replacer). The code is clear apart from the replacer implementation, which has to use the charAt idiom instead of foreach to iterate the string without the defensive copy, and has inline code to escape the escape character itself as well as the quote character. If I was doing it over again, I would try to estimate and pass an initial capacity for the StringBuilder, and see if I really have to break the append chain in order to pass that same StringBuilder to the replacer instead of letting it create its own. I would also change the "TODO use guava joiner" comment to just say *see* guava joiner and see StringJoiner since java 1.8 :( It's more than persuasive enough to drive me to keep trying to pass on my doubts in this forum, but I would rather spare you the javadocs and implementation. Phil To quote Joshua Bloch on good API design: 'When in doubt leave it out' (or 'You can always add, but you can never remove'). On 31 Aug 2013, at 23:13, Mike Duigou <mike.duigou@oracle.com> wrote: On Aug 30 2013, at 23:40 , Philip Hodges wrote:
...
Why is there such a bandwagon rolling for this "convenience" feature?
Perhaps others just don't agree with you. The choice of functionality offered in the JDK 8 StringJoiner was not arbitrary or haphazardly chosen. If it doesn't meet your particular needs, I am sorry to hear that. Our belief as the proposers is that it offers a reasonable mix of functionality and simplicity to cover useful number of requirements. Our intent was certainly not to create the kitchen magician [1] of string joiners that attempted to incorporate every conceivable option. In particular your concerns about escaping are out of scope for the joiner in part because Collection<String> strings; String concat = strings.stream().map(MyUtils::escape).collect(Collections.joining(",")); seems like an adequate and flexible solution to most people.
We already have joiners in apache commons and guava.
This is the reason that StringJoiner was considered for JDK--that many others were "rolling their own".
At first I thought they were cool. Then I tried to use them in anger. And was forced to roll my own.
I would encourage you to share your implementation or the javadocs as grist for discussion. An actual alternative is the *only* thing that is likely to be persuasive.
That can't be right.
Sometimes it is. The JDK doesn't make any attempt to satisfy everyone (or anyone). We all end up writing lots of non-business logic utility functions to bridge gaps in what JDK offers. This is normal. And, if it turns out to be something lots of people need then it's entirely possible that it could be added to the JDK. Mike [1] https://www.youtube.com/watch?v=cGVG9xLHb84
A more elaborate offically blessed feature that only does half the job is worse than useless. Without the extra complex ability to detect or avoid collisions it is neither "nice", nor a "Good Thing".
Phil
http://www.techempower.com/blog/2013/03/26/everything-about-java-8/
"StringJoiner and String.join(...) are long, long overdue. They are so long overdue that the vast majority of Java developers likely have already written or have found utilities for joining strings, but it is nice for the JDK to finally provide this itself. Everyone has encountered situations where joining strings is required, and it is a Good Thing™ that we can now express that through a standard API that every Java developer (eventually) will know. "
On 2013-07-23 22:09, Mike Duigou wrote:
On Jul 23 2013, at 12:43 , ph wrote:
didn't see delimiter collision mentioned anywhere - are you really offering an interface that only joins a list of entries without escaping or quoting or even checking for separators (or escape or quote sequences) that might occur in the entries?
Correct. StringJoiner makes no effort to address escaping/quoting. Doing so would add a lot of complexity that would not useful interesting to most users and probably still wouldn't satisfy everyone.
If you wish some form of escaping or quoting you can pre-processed entries however you like before joining them.
Mike
Please please please drop StringJoiner from Java 1.8 before it is too late. It is not needed. It does not add value. It is an embarrassment. We did without it for years. It is not long long overdue. We do not need it now. It does not need to be in the very first Java 1.8 release. Try leaving it out, and see if people even complain. Most people won't even notice it, let alone be ready to misuse it straight away. String.join is slower than Arrays.toString() or Collections.toString(). Test failed. String.join(delimiter, iterable) needs extra code to call toString() if elements are not already CharSequence. Convenience? Test failed. StringJoiner does not force or even remind the developer to consider delimiter collisions. Test failed. It is a perfect complement for split: the hard to use interface that can't split the joined input reliably because it might have too many delimiters. It will be the newest star on java antipattern web sites. A good place would be just after the item about not "Assembling XML with String operations" because the text parts might contain special characters. Even if it ever did appear that StringJoiner and String.join did what we wanted, I would still much rather roll my own than use the new StringJoiner in the library. Then I could set the initial capacity of the StringBuilder, skip the defensive copies and null checks, and above all, introduce some extra mandatory method call to force the developer to tell the compiler what is being done about delimiter collisions, even if it is to add an assert that there are no delimiters in the added CharSequences, or to waive that check because numbers never never contain comma delimiters (hold on ... better check the locale to see if comma is used as a decimal point or grouping character after all). I came across this non-justification [with my additions] on a blog: "StringJoiner and String.join(...) are long, long overdue. They are so long overdue that the vast majority of Java developers likely have already written or have found [or have had to mend] utilities for joining strings [to make an unsplittable delimiter collision], but it is nice [or at least well-meant] for the JDK to finally provide this itself. Everyone has encountered situations where joining strings is required [and they forgot all about escaping, or should have used a PreparedStatement instead], and it is a Good Thing™ that we can now express that [bug opportunity] through a standard API that every [no, just some] Java developer (eventually) will know [of, but not actually use, and even if they do they will need an IDE to tell them that the prefix comes *after* the delimiter]."
To quote Joshua Bloch on good API design: 'When in doubt leave it out' (or 'You can always add, but you can never remove')
On 31 Aug 2013, at 23:13, Mike Duigou <mike.duigou@oracle.com> wrote:
On Aug 30 2013, at 23:40 , Philip Hodges wrote:
...
Why is there such a bandwagon rolling for this "convenience" feature?
Perhaps others just don't agree with you. The choice of functionality offered in the JDK 8 StringJoiner was not arbitrary or haphazardly chosen. If it doesn't meet your particular needs, I am sorry to hear that. Our belief as the proposers is that it offers a reasonable mix of functionality and simplicity to cover useful number of requirements. Our intent was certainly not to create the kitchen magician [1] of string joiners that attempted to incorporate every conceivable option. In particular your concerns about escaping are out of scope for the joiner in part because
Collection<String> strings;
String concat = strings.stream().map(MyUtils::escape).collect(Collections.joining(","));
seems like an adequate and flexible solution to most people.
We already have joiners in apache commons and guava.
This is the reason that StringJoiner was considered for JDK--that many others were "rolling their own".
At first I thought they were cool. Then I tried to use them in anger. And was forced to roll my own.
I would encourage you to share your implementation or the javadocs as grist for discussion. An actual alternative is the *only* thing that is likely to be persuasive.
That can't be right.
Sometimes it is. The JDK doesn't make any attempt to satisfy everyone (or anyone). We all end up writing lots of non-business logic utility functions to bridge gaps in what JDK offers. This is normal. And, if it turns out to be something lots of people need then it's entirely possible that it could be added to the JDK.
Mike
[1] https://www.youtube.com/watch?v=cGVG9xLHb84
A more elaborate offically blessed feature that only does half the job is worse than useless. Without the extra complex ability to detect or avoid collisions it is neither "nice", nor a "Good Thing".
Phil
http://www.techempower.com/blog/2013/03/26/everything-about-java-8/
"StringJoiner and String.join(...) are long, long overdue. They are so long overdue that the vast majority of Java developers likely have already written or have found utilities for joining strings, but it is nice for the JDK to finally provide this itself. Everyone has encountered situations where joining strings is required, and it is a Good Thing™ that we can now express that through a standard API that every Java developer (eventually) will know. "
On 2013-07-23 22:09, Mike Duigou wrote:
On Jul 23 2013, at 12:43 , ph wrote:
didn't see delimiter collision mentioned anywhere - are you really offering an interface that only joins a list of entries without escaping or quoting or even checking for separators (or escape or quote sequences) that might occur in the entries?
Correct. StringJoiner makes no effort to address escaping/quoting. Doing so would add a lot of complexity that would not useful interesting to most users and probably still wouldn't satisfy everyone.
If you wish some form of escaping or quoting you can pre-processed entries however you like before joining them.
Mike
On Jan 26 2014, at 17:12 , Philip Hodges <philip.hodges@bluewin.ch> wrote:
Please please please drop StringJoiner from Java 1.8 before it is too late.
It is well past "too late". The API has been frozen since August for all but the most exceptional cases.
At first I thought they were cool. Then I tried to use them in anger. And was forced to roll my own.
I would encourage you to share your implementation or the javadocs as grist for discussion. An actual alternative is the *only* thing that is likely to be persuasive.
You seem to be very much in the minority on this issue by the silence from other responders on this list and elsewhere. The concerns you've highlighted with regard to delimiter management, while certainly valid, have not been of sufficient concern to suggest a change in the proposal. The prediction that using StringJoiner will become Java's sprintf seems unlikely to be be true. The reception we've seen thus far for StringJoiner has been otherwise exclusively enthusiastic and positive. Alternatives, refinements and counterproposals are always welcome in the discussion. Doomsaying unfortunately adds little. Mike
I did not predict that it would be a "sprintf". It's not going to be consistently misused anything like so frequently. I compared it to the unescaped XML generation antipattern. I have not seen any technical justifications whatsoever so far, just inexplicable enthusiasm. It is like giving young drivers a faster car with no seat belts. The trouble is, unlike in Google guava, this is the JDK, you can't just drop flawed beta interfaces two versions later. All you can do is add them to static review tool blacklists and deprecate them to cause a warning at compile time, and hope that people will give them a wide berth. Even if you hide them in an unoffical sun.com package. By the way, I'm looking forward to the proprietary Base64 being changed to simply call the new official one. You can't even modify an equals method in an undocumented reflection implementation in 1.7.0.51 without breaking production applications. Wow. I am raising doubt and you are ignoring it. Don't you have the guts to say whoa, stop the bandwagon, there could just be a real problem with this, it really will make it easier to create bugs without any warning from the compiler and without making anyone's code better *in any way at all*? I am picking on String.join precisely because I have seen way too many delimiter collision bugs, not just in Java but in several other languages, and because this interface perpetuates a real world problem by preventing future interface changes to detect them. [I am not always a doomsayer. For example, I am not picking on JSR310 because Date and Calendar and SimpleDateFormat are well known disaster areas and the people working on their replacement have a deep understanding of the issues, have really taken their time, and nothing whatsoever that I have read about it jumped out at me and said this is *wrong*. It might not even be completely perfect. But I am confident it will be so much better than what we have now, and it's a shame that I won't have time to migrate existing apps to it.] The counterproposal, no, proposal refinement, wafting around inside my head, is to somehow compel programmers to make just one more method call before that final toString(). It will be difficult to find good names, especially ones that will be understood by programmers for whom English is not a first language. Something like: String.join(delimiter, joinables).assertNoUnescapedDelimiters().toString(); String.join(delimiter, joinables).neverMindDelimiterCollisions().toString(); String.join(delimiter, joinables).promiseNoUnescapedDelimiters().toString(); String.join(delimiter, joinables).escapeDelimiters(escapeChar).toString(); String.join(delimiter, joinables).quoteElements(quoteChar).toString(); The vital thing is that String.join has to return an unjoinable, that needs an adapter method to make it safely joinable. If you get that right, then we can forgive this first shot being a little slow, and enjoy the triumph of CharSequence over immutability. Yes, ultimately the goal should be to add full support for at least the most popular csv generation recipes. I'm really sorry I couldn't carry on arguing the case before August. As a minority, I only have one person's quota of energy. I will try to get some more people to look at it. On 27 Jan 2014, at 18:44, Mike Duigou <mike.duigou@oracle.com> wrote:
On Jan 26 2014, at 17:12 , Philip Hodges <philip.hodges@bluewin.ch> wrote:
Please please please drop StringJoiner from Java 1.8 before it is too late.
It is well past "too late". The API has been frozen since August for all but the most exceptional cases.
At first I thought they were cool. Then I tried to use them in anger. And was forced to roll my own.
I would encourage you to share your implementation or the javadocs as grist for discussion. An actual alternative is the *only* thing that is likely to be persuasive.
You seem to be very much in the minority on this issue by the silence from other responders on this list and elsewhere. The concerns you've highlighted with regard to delimiter management, while certainly valid, have not been of sufficient concern to suggest a change in the proposal. The prediction that using StringJoiner will become Java's sprintf seems unlikely to be be true. The reception we've seen thus far for StringJoiner has been otherwise exclusively enthusiastic and positive.
Alternatives, refinements and counterproposals are always welcome in the discussion. Doomsaying unfortunately adds little.
Mike
On 01/27/2014 12:31 PM, Philip Hodges wrote:
I did not predict that it would be a "sprintf". It's not going to be consistently misused anything like so frequently. I compared it to the unescaped XML generation antipattern.
I have not seen any technical justifications whatsoever so far, just inexplicable enthusiasm.
It is like giving young drivers a faster car with no seat belts.
As Mike had said, delimiter collision is a valid concern, and I think your metaphore is quite vivid. Anyhow, there is need for fast and simply join operation, and this is what JDK is intend to provide. Nothing prohibit developer to - choose a better delimiter for the data - escape the element before sending to joiner Since we expect more complicate use case is more likely to be used in stream API, such escaping can be easily done by adding a map step. elements.stream() .map(escapeFunction) .collect(joining()) Cheers, Henry
String is probably the most commonly used class in the whole of Java. Please please please do not pollute it with this dangerous new method. Your Java-is-cool example is ridiculously trivial, of no practical use whatsoever, and absolutely *not* cool. How about a proper test where you have to double up the delimiter if it occurs in an element: assertEquals("'Java','isn''t','perfect'", "'"+String.join("','", javaIsNotPerfect)+"'"); I now have a working proof of concept, where newJoiner returns an UnsplittableJoiner which requires a delimiter handler to convert it back into a Joiner. Actually my joiner is an ObjectJoiner, unlike your StringJoiner that is just a CharSequenceJoiner, unlike AbstractCollections.toString and Arrays.toString which can join any elements that have a toString method. Please have the guts to pull String.join from the release now, before it becomes an antipattern, and you have to deprecate it. And tell everybody exactly why. Seriously. On 27 Jan 2014, at 21:31, Philip Hodges <philip.hodges@bluewin.ch> wrote:
I did not predict that it would be a "sprintf". It's not going to be consistently misused anything like so frequently. I compared it to the unescaped XML generation antipattern.
I have not seen any technical justifications whatsoever so far, just inexplicable enthusiasm.
It is like giving young drivers a faster car with no seat belts.
The trouble is, unlike in Google guava, this is the JDK, you can't just drop flawed beta interfaces two versions later. All you can do is add them to static review tool blacklists and deprecate them to cause a warning at compile time, and hope that people will give them a wide berth. Even if you hide them in an unoffical sun.com package. By the way, I'm looking forward to the proprietary Base64 being changed to simply call the new official one. You can't even modify an equals method in an undocumented reflection implementation in 1.7.0.51 without breaking production applications. Wow.
I am raising doubt and you are ignoring it. Don't you have the guts to say whoa, stop the bandwagon, there could just be a real problem with this, it really will make it easier to create bugs without any warning from the compiler and without making anyone's code better *in any way at all*?
I am picking on String.join precisely because I have seen way too many delimiter collision bugs, not just in Java but in several other languages, and because this interface perpetuates a real world problem by preventing future interface changes to detect them.
[I am not always a doomsayer. For example, I am not picking on JSR310 because Date and Calendar and SimpleDateFormat are well known disaster areas and the people working on their replacement have a deep understanding of the issues, have really taken their time, and nothing whatsoever that I have read about it jumped out at me and said this is *wrong*. It might not even be completely perfect. But I am confident it will be so much better than what we have now, and it's a shame that I won't have time to migrate existing apps to it.]
The counterproposal, no, proposal refinement, wafting around inside my head, is to somehow compel programmers to make just one more method call before that final toString(). It will be difficult to find good names, especially ones that will be understood by programmers for whom English is not a first language. Something like:
String.join(delimiter, joinables).assertNoUnescapedDelimiters().toString(); String.join(delimiter, joinables).neverMindDelimiterCollisions().toString(); String.join(delimiter, joinables).promiseNoUnescapedDelimiters().toString(); String.join(delimiter, joinables).escapeDelimiters(escapeChar).toString(); String.join(delimiter, joinables).quoteElements(quoteChar).toString();
The vital thing is that String.join has to return an unjoinable, that needs an adapter method to make it safely joinable. If you get that right, then we can forgive this first shot being a little slow, and enjoy the triumph of CharSequence over immutability.
Yes, ultimately the goal should be to add full support for at least the most popular csv generation recipes.
I'm really sorry I couldn't carry on arguing the case before August. As a minority, I only have one person's quota of energy. I will try to get some more people to look at it.
On 27 Jan 2014, at 18:44, Mike Duigou <mike.duigou@oracle.com> wrote:
On Jan 26 2014, at 17:12 , Philip Hodges <philip.hodges@bluewin.ch> wrote:
Please please please drop StringJoiner from Java 1.8 before it is too late.
It is well past "too late". The API has been frozen since August for all but the most exceptional cases.
At first I thought they were cool. Then I tried to use them in anger. And was forced to roll my own.
I would encourage you to share your implementation or the javadocs as grist for discussion. An actual alternative is the *only* thing that is likely to be persuasive.
You seem to be very much in the minority on this issue by the silence from other responders on this list and elsewhere. The concerns you've highlighted with regard to delimiter management, while certainly valid, have not been of sufficient concern to suggest a change in the proposal. The prediction that using StringJoiner will become Java's sprintf seems unlikely to be be true. The reception we've seen thus far for StringJoiner has been otherwise exclusively enthusiastic and positive.
Alternatives, refinements and counterproposals are always welcome in the discussion. Doomsaying unfortunately adds little.
Mike
Hi Philip, Am 27.01.2014 02:12, schrieb Philip Hodges:
Please please please drop StringJoiner from Java 1.8 before it is too late. I have not seen any technical justifications whatsoever so far, just inexplicable enthusiasm.
It is like giving young drivers a faster car with no seat belts.
+++1 I'm also with you with all your arguments against this API.
I'm really sorry I couldn't carry on arguing the case before August. As a minority, I only have one person's quota of energy. I will try to get some more people to look at it.
You are not alone. Although I missed the delimiter/escaping problem those days, it was April 18 I posted my different concerns about StringJoiner in this list: <<<<<<<<<< I'm wondering, that StringJoiner has some logic for pre/suffix, but nothing to loop the elements themselves To me, StringJoiner is a useless complicated box around StringBuilder, and imagine, someone needs thread-safety. It also slows down performance, as it needs additional instances and additional class to be loaded (critical at VM startup). Instead please add to StringBuilder and StringBuffer: append(CharSequence... elements); append(char delimiter, CharSequence... elements); append(char delimiter, Iterable<? extends CharSequence> elements); cut(int len); // removes len chars at the end of the sequence optional: append(CharSequence delimiter, CharSequence... elements); append(CharSequence delimiter, Iterable<? extends CharSequence> elements); For performance reasons, append() should always append the trailing delimiter, which could be cut at the end. It's questionable, if class string needs a static (=no relation to an existing string in contrast to non-static split()) join method, as it seduces to "[" + String.join(...) + "]" which needs some effort from javac side to optimize to a single StringBuilder task. IMO we better had StringBuilder.join(...), so javac could easily optimize to: new StringBuilder().append('[').append(',', someStrings).cut(1).append(']').toString() THe current proposed implementation has: 1 class with 7 methods 2 additional methods in String To cover the same functionality, above approach basically only needs 2 additional methods in StringBuilder, has better performance, so what is complicated on that?
>>>>
Am 27.01.2014 18:44, schrieb Mike Duigou:
The reception we've seen thus far for StringJoiner has been otherwise exclusively enthusiastic and positive.
Where are those people, they don't speak up in this list, seem to don't know about the performance issues and the traps which are mentioned here. We don't know how they will deal with the problems in practical if they occur in real world. On the other hand, the "doomsayers" also could refer to others out there which see no win in this API. -Ulf
participants (12)
-
Alan Bateman
-
Brian Goetz
-
Henry Jen
-
Jim Gish
-
Martin Buchholz
-
Mike Duigou
-
ph
-
Philip Hodges
-
Remi Forax
-
Roger Riggs
-
Steven Schlansker
-
Ulf Zibis