Please review - 6984084 - String repeat()

[Talden]

It's not clear to me why this is implemented in terms of StringJoiner
at all.  In fact, using StringJoiner here seems to enforce a
sub-optimal allocation-pattern for no obvious complexity benefit.

I'm also with you, Mike, that n < 0 should probably be an IAE.  The
empty String seems surprising in both the String and StringBuilder
cases (and especially my StringBuilder.appendRepeated case).

Assuming the original contract though, a solution might be...

// String.java
public String repeat( int n ) {
  if(n < 0) return "";
  if(n == 0) return this;
  return new StringBuilder(length() * n).appendRepeated(this, n).toString();
}

And have both repeat(int) and appendRepeated(CharSequence, int) on StringBuilder

// AbstractStringBuilder.java
AbstractStringBuilder appendRepeated(final CharSequence s, final int n) {
  if(n <= 0) {
    if(n < 0) setLength(0);
    return this;
  }

  ensureCapacityInternal(s.length() * n + count);

  for(int i = 0; i < n; n++) {
    append(s);
  }

  return this;
}

AbstractStringBuilder repeat(final int n) {
  if(n <= 0) {
    if(n < 0) setLength(0);
    return this;
  }

  return appendRepeated(toString(), n - 1);
}

...in retrospect I think there are probably additional n==1 paths that
can be made cheaper too.

--
Aaron Scott-Boddendijk


On Thu, Aug 16, 2012 at 7:35 AM, Mike Duigou <mike.duigou at oracle.com> wrote:
> webrev::
>
> - I use "duke" (which is a valid OpenJDK id used for admin activities) as my reviewed-by reviewer until I have real reviewers.
>
> - my OpenJDK id is mduigou (your webrev is missing a "u").
>
> StringJoiner::
>
> - Why is a temporary introduced into add(CharSequence?)
>
> - The test n > 0 is unnecessary as the loop condition will have the same effect.
>
> - Why accept and ignore n <= 0 and not instead throw a IAE? (I always suspect that this will hide errors).
>
> AbstractStringBuilder::
>
> - behaviour for negative n is rather surprising to me. I'd rather just see an IAE.
>
> - Rather than using append and n-1 you might want to use the "set value" phrasing. ie.
> "Set the value of this StringBuilder to be n concatenated copies of the current value."
>
>
> String::
>
> - behaviour for negative n is rather surprising to me. I'd rather just see an IAE.
>
> - Rather than using append and n-1 you might want to use the "set value" phrasing. ie.
> "Return a String consisting of n concatenated copies of this String."
>
>
> On Aug 15 2012, at 11:41 , Jim Gish wrote:
>
>> Please review http://cr.openjdk.java.net/~jgish/6984084-StringRepeat/
>>
>> This in response to an old RFE to add either a new constructor or a
>> method to repeat a string, such as "*".repeat(5) resutling in "*****"
>>
>> There is nothing particularly lambda related in here other then the fact
>> that under the covers I'm using StringJoiner.
>>
>> I'm happy with the name "repeat" but question the "addNcopies" method in
>> AbstractStringBuilder -- suggestions welcome.
>>
>> Thanks,
>>    Jim
>>
>> -- 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
>>
>
>