Add getChars to CharSequence

[Ulf Zibis]

Hi Martin,  (cc'd core-libs-dev)

Am 23.04.2013 21:36, schrieb Martin Buchholz:
> Hi Ulf!
>
>
> On Mon, Apr 22, 2013 at 9:11 PM, Ulf Zibis <Ulf.Zibis at cosoco.de <mailto:Ulf.Zibis at cosoco.de>> wrote:
>
>     Am 22.04.2013 21:45, schrieb Martin Buchholz:
>
>         Another preliminary webrev is out at
>         http://cr.openjdk.java.net/~martin/webrevs/openjdk8/getChars/
>         <http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk8/getChars/>
>         <http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk8/getChars/>
>
>     This looks great, but AbstractStringBuilder could be again simplified ...
>
>     1. What is the difference between replaceOutOfBounds() and substringOutOfBounds() ? ;-)
>
>
> detail message is different to try to match the parameter names (most of the time...)

Hm, maybe I'm blind, I can't see any difference, even in the parameter names.

>     2. naming suggestion:
>         outOfBoundsMsg() --> outOfSrcBoundsMsg()
>         replaceOutOfBounds(), substringOutOfBounds() --> outOfDstBoundsMsg()
>

Hm, I'm additionally "concerned" about that in one case you add the suffix "Msg", but not in the others.

>
>     3. You could rename the parameters of getChars to:
>         getChars(int start, int end, char[] dst, int dstStart)
>         Then you only need one method: outOfBoundsMsg() !!
>         Note that append(CharSequence s, int start, int end), calling getChars(), also uses start,
>     end.
>
>
> If a method has a "src" and "dst", then it makes sense to make that clear in the parameter names, 
> but if it only has a "src", then it makes sense to leave out the prefix "src" in the parameter names.

Well, if one invokes append(CharSequence s, int start, int end), then IMO it's confusing to see 
"srcBegin" + "srcEnd" in the exception message. Why isn't this possible to change as you stated below?
Instead "dst" you could also use "result" or "drain", and instead "dstStart", "dstOffset" or just 
"offset". The parameters order would make it clear enough that "offset" is meant for "dst".

>  It's annoying to use the words "begin" and "start" interchangeably.  I prefer "start", but it's 
> probably not possible to fix this now.

That's exactly, what I wanted to say. I think it's up to you, because it's a new introduced method, 
to use "start" rather than "begin", as I too would prefer it.

>     4. Instead:
>             if (start < 0 || start > end || end > count)
>         you could write:
>             if (start < 0 || end - start < 0 || end > count)
>         because result of end - start could be reused later.
>
>
> Yeah, and we can do even better:
>
>     public void getChars(int srcBegin, int srcEnd, char[] dst, int dstBegin) {
>         int length = srcEnd - srcBegin;
>         if ((srcBegin | length | (count - srcEnd)) < 0)
>             throw new StringIndexOutOfBoundsException
>                 (outOfBoundsMsg(srcBegin, srcEnd));
>         System.arraycopy(value, srcBegin, dst, dstBegin, length);
>     }

Yep, great discovery ! I guess, there are many places in the JDK, where this would apply.

>     6. insert() methods should likewise throw SIOOBE instead IOOBE.
>         Then likewise outOfBoundsMsg() could be used.
>
>
> I'm willing to change exception detail, but not the actual class of the exception thrown.

Hm, in my feeling it was by mistake, to inconsistently throw IOOBE instead SIOOBE. IMO it wouldn't 
hurt to throw an SIOOBE, as it's derived from IOOBE, so catching for IOOBE would still work.

-Ulf