Hi Ulf!
On Mon, Apr 22, 2013 at 9:11 PM, Ulf Zibis <ulf.zi...@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/~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...) > 2. naming suggestion: > outOfBoundsMsg() --> outOfSrcBoundsMsg() > replaceOutOfBounds(), substringOutOfBounds() --> outOfDstBoundsMsg() > > 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. It's annoying to use the words "begin" and "start" interchangeably. I prefer "start", but it's probably not possible to fix this now. > 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); } > 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.