hg: lambda/lambda/jdk: 2 new changesets

[Roel Spilker]

B) Also, in several places in the Javadoc, it says something like: The
length of the StringJoiner value, equivalent to toString().length().
However, neither StringJoiner nor toString are final. Therefore it is
impossible to guarantee this.

Roel

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

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