Feedback on the implementation of StringJoiner

[Henry Jen]

On 12/12/2012 12:39 PM, Roel Spilker wrote:
> And subSequence could do a slightly better jo
> 
>  if (start < value.length()) {
>             return new StringBuilder(value.subSequence(start, value.length()))
>                         .append(suffix.subSequence(0, end - value.length()))
>                         .toString();
> 
> 
> is IMHO less performant than:
> 
>  if (start < value.length()) {
>             return new StringBuilder(end - start)
> 
>                         .append(value.subSequence(start, value.length()))
> 
> .append(suffix.subSequence(0, end - value.length())) .toString();
> 
>

Agree.

Anyway, the implementation no longer implement CharSequence, thus more
code removed.

> 
> On Wed, Dec 12, 2012 at 9:34 PM, Roel Spilker <r.spilker at gmail.com> wrote:
> 
>> Oh, and please remove the string concatenation from the constructor. Since
>> prefix and suffix are stored separately and setEmptyOutput does not accept
>> null values, a null-check on the field can be used to calculate the
>> emptyOutput when it is needed. I think the common case is that the
>> emptyOutput value is not read at all.

I don't see much gain here, it make sense to me to have a proper initial
value and it's not likely to be a bottleneck for performance or memory
print.

We do keep length() API just in case someone would like to know before
actually convert to string. Have a proper value for emptyOutput helps
not to duplicate code.

We can extract the logic to an internal private method, that would be an
overhead for each call.

>>
>>
>> On Wed, Dec 12, 2012 at 9:27 PM, Roel Spilker <r.spilker at gmail.com> wrote:
>>
>>> Well, a lot of code was removed, what is in my opinion always a good
>>> thing.
>>>
>>> Apart from the comments already mentioned, I have one other point. The
>>> JavaDoc of length(), charAt(int) and subSequence(int, int) still define
>>> their behavior as being equivalent to the same operation on the toString()
>>> result. However, the class isn't final. I don't think it is a promise that
>>> should be made, because it puts quite a burden on a subclass. My preferred
>>> solution would be to make the class final. However, I don't know if that is
>>> an option.

I believe it is reasonable to have contract state the expect value,
which is proper value based on the string representation at the moment.

You have a good point whether we should mention toString(), as the
contract specified that toString() should return the string
representation of the value, they are equivalent.

Cheers,
Henry