hg: lambda/lambda/jdk: 2 new changesets

[Roel Spilker]

I made a mistake in A

`prepareBuilder` should be called inside the loop, but should probably have
a different name, like `prepareForAddition`

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
>>
>>
>