On Jun 19, 2014, at 7:39 PM, Claes Redestad <claes.redes...@oracle.com> wrote: > Hi, > > an updated webrev with reworked, public methods is available here: > http://cr.openjdk.java.net/~redestad/8041972/webrev.8/ > > Reviews are yet again appreciated! >
I think "if (s == null)" or "Objects.requireNonNull(s)" is preferable to s.getClass(). (I am informed by those more knowledgeable than i that the latter also has poorer performance in the client compiler.) Related to the above, it might be preferable to retain the existing semantics of throwing NumberFormatException on a null string value, since someone might switch between parseInt(String ) and parseInt(String, int, int, int) and the null will then slip through the catch of NumberFormatException. You could just use IndexOutOfBoundsException for the case of beginIndex < 0 and beginIndex >= endIndex and endIndex > s.length(), as is similarly the case for String.substring. Again, like previously, switching between parseInt(String, int, int) parseInt(String, int, int, int) requires no additional catching. You might want to add a comment in the code that some IndexOutOfBoundsException exceptions are implicit via calls to s.charAt (i did a double take before realizing :-) ). Integer.requireNonEmpty could be a static package private method on String (or perhaps even static public, it seems useful), plus i would not bother with the static import in this case. In Integer.java#643 you can combine the if statements like you have done for the equivalent method on Long. Hth, Paul.
