Feedback on the implementation of StringJoiner

[Roel Spilker]

Also, the javadoc on setEmptyOutput explicitly states that the Joiner is
only considered empty when the add has not been called, and not when the
resulting String would be empty. This conflicts with the current
immplementation of toString. The proposed null-value would fix this.

Roel



On Fri, Dec 14, 2012 at 10:31 AM, Roel Spilker <r.spilker at gmail.com> wrote:

> It is already pushed to the lambda repo:
> http://hg.openjdk.java.net/lambda/lambda/jdk/annotate/6fb8cefc938c/src/share/classes/java/util/StringJoiner.java
>
> I'm not sure that that's correct. If the prefix and the first added
> element are both empty and the infix is not, I would expect the resulting
> String to start with the infix, but in this implementation it starts with
> the first added non-empty CharSequence. So I think we still need the
> somethingAdded field, or need to initialize the value field with null and
> construct the value when the first value is added.
>
> Also, the lambda implementation still does the string concatenation in the
> constructor.
>
> Roel
>
>
>
> On Thu, Dec 13, 2012 at 8:18 PM, Henry Jen <henry.jen at oracle.com> wrote:
>
>> On 12/13/2012 01:37 AM, Remi Forax wrote:
>> > Hi Henry,
>> > if we don't change the implementation of StringJoiner now (see my mail
>> > about my inability to test the stream pipeline),
>> > there are at least two things that can be changed in your current
>> > implementation.
>> >
>>
>> We don't change as I explained to you, save an intermediate allocate of
>> char[] and two copies for a list and copy of references is not necessary
>> better.
>>
>> > All the static final String can be removed because the string are
>> > interned so there is no need to store then in a static fields.
>> > Even if you think the code is more clear with that and you may be right,
>> > it's not a standard practice, the only code that does that is
>> > String.java (as far as I know).
>> >
>> > You don't need the field 'somethingAdded' because you can test the
>> > length of the StringBuilder (value).
>> >
>>
>> Agree.
>>
>> Cheers,
>> Henry
>>
>>
>>
>