hg: lambda/lambda/jdk: 2 new changesets

[Jim Gish]

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