Feedback on the implementation of StringJoiner

[Remi Forax]

On 12/10/2012 05:26 PM, Henry Jen wrote:
> Hi Roel,
>
> I am doing some changes to adapt your feedbacks and trying to have minimum but enough APIs, latest proposed can be found at
>
> http://cr.openjdk.java.net/~henryjen/lambda/StringJoiner.1/webrev/
>
> - remove add(CharSequence...) and add(char[]) from StringJoiner
> - make StringJoiner(CharSequence) public and return StringJoiner. This
> should be enough to cover cases where stream not available and we just
> want to concat a few items. Also this is the only form we need for
> String.join()
> - Remove [AbstractStringBuilder/StringBuilder/StringBuffer].join(), in
> favor of .append(String.join(..)). Essentially the same and API is more
> comprehensive.
> - Disallow null container(Iterable, Stream, varargs, etc), tolerate
> container to have null content.
>
> Feel free to send your feedbacks. Thanks for the review.

Henry, I'm sorry but this implementation is just horrible ...

A StringJoiner is something which is able to concatenate String but 
wait, it's also a CharSequence too.
Why using inheritance over composition here, if you want a CharSequence, 
why not having a method toCharSequence()
I don't see myself why StringJoiner need to return a CharSequence given 
that String is a CharSequence.

CharSequence is interresting when you take it as parameter but when you 
return it (or worst implement it),
what is the gain ?

So the conception is awful, and the implementation is awful too.

Let say I just want to add a String and call toString, add will put the 
String into the StringBuilder, Ok and toString(), surprise, toString 
execute value.toString() + suffix, + is implemented by creating a new 
StringBuilder and adding the two strings, so the code take the 
StringBuilder value, create a new String from it (the .toString()), 
create a new StringBuilder (the +) append the freshly created String 
into the StringBuilder, append the suffix into the StringBuilder and 
recreate a new String. If you count, you have all the chars of the 
StringBuilder that are copied 3 times.

The right way to implement it is to use an ArrayList (or a dedicated 
list as the one recently puhed in the lambda repo) to store all the 
CharSequence that are added, and to also maintain the total number of 
chars, when toString is called, allocate one new char array with the 
size (total number of chars + (list.size() - 1) *  delimiterSize + 
prefix and suffix size and then crawle the ArrayList and store each 
CharSequence at the right place.

>
> Cheers,
> Henry

Rémi

>
> On Dec 10, 2012, at 1:59 AM, Roel Spilker <r.spilker at gmail.com> wrote:
>
>> I understand. If I can help in any way, please let me know. I did sign an
>> OCA 10 days ago, however, I don't know if it has been processed.
>>
>>
>> On Fri, Nov 30, 2012 at 4:58 PM, Jim Gish <jim.gish at oracle.com> wrote:
>>
>>> Hi Roel,
>>>
>>> I'll take a look.  Sorry for the delay -- got pulled into other things.
>>>
>>> Jim
>>>
>>> On 11/28/2012 02:41 PM, Roel Spilker wrote:
>>>
>>>> Hi all,
>>>>
>>>> On June 25 I sent some feedback on the implementation of StringJoiner.
>>>> Most
>>>> of it is still relevant, none of it is addressed. Is there anything I can
>>>> do to get the implementation to a higher level?
>>>>
>>>> Roel
>>>>
>>>> Thread containing the feedback
>>>> http://mail.openjdk.java.net/**pipermail/lambda-dev/2012-**
>>>> June/005078.html<http://mail.openjdk.java.net/pipermail/lambda-dev/2012-June/005078.html>
>>>>
>>>> Link to the current implementation
>>>> http://hg.openjdk.java.net/**lambda/lambda/jdk/file/**
>>>> 83923cc50252/src/share/**classes/java/util/**StringJoiner.java<http://hg.openjdk.java.net/lambda/lambda/jdk/file/83923cc50252/src/share/classes/java/util/StringJoiner.java>
>>>>
>>>>
>>> --
>>> 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 at oracle.com
>>>
>>>
>