hg: lambda/lambda/jdk: 2 new changesets

Mon Jun 25 11:20:56 PDT 2012

On the API level:

1) addAll/add(CharSequence...) will append "null" to `value` if the
parameter is null; this is unexpected since we're joining elements, and
there are no null elements. I would suggest either throwing an NPE or treat
it as an empty list/array (i.e do nothing)

2) How does this class compile? It implements Fillable<CharSequence> but
does not have a method `boolean add(CharSequence )`. It does have
`StringJoiner add(CharSequence...)` though. What is the point of making a
method both chaining AND varargs? Also, getting rid of the varargs means
that I can actually add a single String without having to create an array
instance every time.

3) Can the return type of addAll be made StringJoiner? That would be more
consistent with the rest of the API.

4) Should there be a method `StringJoiner add(Object)` and possibly also
methods for the primitive types? This would make it more consistent with
StringBuilder, PrintStream etc.

Roel

On Mon, Jun 25, 2012 at 7:54 PM, Roel Spilker <r.spilker at gmail.com> wrote:

> Two more things on the current implementation:
>
> 9) The javadoc has some typos: `nullOuput` instead of `nullOutput` or
> even better "null"
>
> A) In addAll the prepareBuilder is invoked inside the loop.
>
> Roel
>
> On Mon, Jun 25, 2012 at 6:14 PM, Jim Gish <jim.gish at oracle.com> wrote:
>
>> Thanks for the feedback, Roel.  I'll take a look at the suggested code
>> changes in a bit.  As far as your enhancement suggestions go for
>> null/empty, I started off with something comparable, but was urged to keep
>> it simple for this initial round.  However, I hope to add these at some
>> point.
>>
>> Thanks,
>>   Jim
>>
>>
>> On 06/25/2012 11:37 AM, Roel Spilker wrote:
>>
>>> Regarding the API, I can also imagine the need/wish for special handling
>>> of
>>> null and empty values.
>>>
>>> /** Treat null values as if the weren't there*/
>>> StringJoiner ignoreNull() {};
>>>
>>> /** Replace null values by the given value*/
>>> StringJoiner useForNull(String nullString) {};
>>>
>>> /** Treat empty values as if the weren't there*/
>>> StringJoiner ignoreEmpty() {};
>>>
>>> /** Replace empty values by the given value*/
>>> StringJoiner useForNull(String emptyString) {};
>>>
>>> Roel
>>>
>>> On Mon, Jun 25, 2012 at 5:23 PM, Roel Spilker<r.spilker at gmail.com>
>>>  wrote:
>>>
>>>  I understand the need, and give feedback on the implementation, assuming
>>>> the current API.
>>>>
>>>> Feedback on the implementation of StringJoiner.java
>>>>
>>>> 1) The field `nullOutput` could become a constant, since there it is
>>>> never
>>>> written to:
>>>> private static final String NULL_OUTPUT = "null";
>>>>
>>>> 2) There is no need for the field `nullOutputChars`. It is only used
>>>> once,
>>>> in add(char[]) . This could easily be rewritten to:
>>>>
>>>> public StringJoiner add(char[] newElement) {
>>>> prepareBuilder();
>>>> if (newElement == null) {
>>>>  value.add(NULL_OUTPUT);
>>>> } else {
>>>> value.add(newElement);
>>>>  }
>>>> return this;
>>>> }
>>>>
>>>> 3) `toString` calls `emptyOutput.toString()`. There is no need for that;
>>>> it is already a String.
>>>>
>>>> 4) The field `prefixLen` can be removed entirely. It is only used in
>>>> prepareBuilder the first time something is added. Actually, I think that
>>>> the code `if (prefixLen>  0)` can be ommitted at all, but at least it
>>>> can
>>>> be replaced by `if (prefix.length()>  0)`
>>>>
>>>> 5) In `subSequence`, instead of creating a StringBuilder using the
>>>> CharSequence constructor, I think it is better to create a
>>>> StringBuilder of
>>>> the right size and then append the subSequences of the `value` and
>>>> `suffix`:
>>>>
>>>> if (start<  value.length()) {
>>>> return new StringBuilder(end - start)
>>>> .append(value.subSequence( start, value.length()))
>>>>  .append(suffix.subSequence( 0, end - value.length());
>>>> }
>>>>
>>>> 6) In the constructor, in the common case there is no need for String
>>>> concatenation:
>>>> this.emptyOutput = this.suffix.isEmpty() ? this.prefix : this.prefix +
>>>> this.suffix;
>>>>
>>>> 7) `asChars`, `asCharArray` and `asCodePoints` be made much simpler by
>>>> calling toString (or a private method toStringInternal, ontaining the
>>>> current toString implementation, since a subclass might have overridden
>>>> toString) and than delegate the call:
>>>>
>>>> public Iterable<Character>  asChars() {
>>>> return toStringInternal().asChars();
>>>> }
>>>>
>>>> public char[] asCharArray() {
>>>>  return toStringInternal().**asCharArray();
>>>> }
>>>>
>>>> public Iterable<Integer>  asCodePoints() {
>>>> return toStringInternal().**asCodePoints();
>>>> }
>>>>
>>>> This also takes care of the current inconsistencies beween `asCharArray`
>>>> and the other two methods.
>>>>
>>>> 8) Possibly get rid of the `prefix` field by adding it directly to the
>>>> `value` field in the constructor:
>>>>
>>>> String prefixValue = prefix.toString();
>>>> this.emptyOutput = this.suffix.isEmpty() ? prefixValue : prefixValue +
>>>> this.suffix;
>>>> this.value = new StringBuilder().append(**prefixValue);
>>>>
>>>> On all code paths, the field `somethingAdded` is checked before reading
>>>> the `value` field.
>>>>
>>>> The method `prepareBuilder` would then become:
>>>>
>>>> if (!somethingAdded) {
>>>>  somethingAdded = true;
>>>> } else {
>>>> value.append(infix);
>>>> }
>>>>
>>>> Removing the field is obviously a good thing. The downside is that the
>>>> contents of the prefix is now always copied to the `value` field, even
>>>> when
>>>> no actual values are added. Then again, I expect the prefix to be empty
>>>> in
>>>> most of the use cases.
>>>>
>>>> Roel
>>>>
>>>>
>>>> On Mon, Jun 25, 2012 at 4:29 PM, Brian Goetz<brian.goetz at oracle.com>**
>>>> wrote:
>>>>
>>>>  This is a reasonable place for such feedback.
>>>>>
>>>>> The motivation for StringJoiner was twofold:
>>>>>  - to address the long-standing RFE for String.join;
>>>>>  - to provide an easy means of joining a stream of strings (e.g.,
>>>>> names =
>>>>> people.map(Person::getName).****into(new StringJoiner(", "))
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 6/25/2012 7:56 AM, Roel Spilker wrote:
>>>>>
>>>>>  I have some feedback on the implementation of StringJoiner. Where and
>>>>>> how
>>>>>> can I give this feedback?
>>>>>>
>>>>>> Roel
>>>>>>
>>>>>> On Sat, Jun 23, 2012 at 3:10 AM,<stuart.marks at oracle.com>   wrote:
>>>>>>
>>>>>>  Changeset: 915296b3c16e
>>>>>>
>>>>>>> Author:    jgish
>>>>>>> Date:      2012-06-20 18:40 -0400
>>>>>>> URL:       http://hg.openjdk.java.net/****lambda/lambda/jdk/rev/**<http://hg.openjdk.java.net/**lambda/lambda/jdk/rev/**>
>>>>>>> 915296b3c16e<http://hg.**openjdk.java.net/lambda/**
>>>>>>> lambda/jdk/rev/915296b3c16e<http://hg.openjdk.java.net/lambda/lambda/jdk/rev/915296b3c16e>
>>>>>>> >
>>>>>>>
>>>>>>>
>>>>>>> 5015163: (str) String merge/join that is the inverse of
>>>>>>> String.split()
>>>>>>> Summary: Convenience methods on String to access the
>>>>>>> java.util.StringJoiner functionality
>>>>>>> Reviewed-by: smarks, briangoetz
>>>>>>>
>>>>>>> ! src/share/classes/java/lang/****AbstractStringBuilder.java
>>>>>>> ! src/share/classes/java/lang/****String.java
>>>>>>> ! src/share/classes/java/lang/****StringBuffer.java
>>>>>>> ! src/share/classes/java/lang/****StringBuilder.java
>>>>>>> ! src/share/classes/java/util/****StringJoiner.java
>>>>>>> ! test-ng/tests/org/openjdk/****tests/java/lang/StringTest.****java
>>>>>>> ! test-ng/tests/org/openjdk/****tests/java/util/****
>>>>>>> FillableStringTest.java
>>>>>>> ! test-ng/tests/org/openjdk/****tests/java/util/****
>>>>>>> StringJoinerTest.java
>>>>>>>
>>>>>>>
>>>>>>> Changeset: 7af64c20ebe7
>>>>>>> Author:    smarks
>>>>>>> Date:      2012-06-22 18:12 -0700
>>>>>>> URL:       http://hg.openjdk.java.net/****lambda/lambda/jdk/rev/**<http://hg.openjdk.java.net/**lambda/lambda/jdk/rev/**>
>>>>>>> 7af64c20ebe7<http://hg.**openjdk.java.net/lambda/**
>>>>>>> lambda/jdk/rev/7af64c20ebe7<http://hg.openjdk.java.net/lambda/lambda/jdk/rev/7af64c20ebe7>
>>>>>>> >
>>>>>>>
>>>>>>> merge
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>> --
>> 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
>>
>>
>