hg: lambda/lambda/jdk: 2 new changesets

[Roel Spilker]

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