Hi Christoph,
I think it's worth adding a small comment to the end of `String tgtStr =
target.toString();`, saying that the null check is implicit, in case people
read the code in the future and get confused as to why only `replacement`
is apparently null-checked. What do you think?
Kind regards,
Jonathan
On 1 March 2017 at 23:47, Christoph Dreis <christoph.dr...@freenet.de>
wrote:
> Hey Vitaly,
>
> > Seems like a good idea. You probably want to null check 'replacement'
> before the bail out as the method is specified as throwing NPE in that case.
>
> Thanks for your feedback. See the adjusted patch below.
>
> ===== PATCH ======
>
> diff --git a/src/java.base/share/classes/java/lang/String.java
> b/src/java.base/share/classes/java/lang/String.java
> --- a/src/java.base/share/classes/java/lang/String.java
> +++ b/src/java.base/share/classes/java/lang/String.java
> @@ -2176,12 +2176,13 @@
> * @since 1.5
> */
> public String replace(CharSequence target, CharSequence replacement) {
> + Objects.requireNonNull(replacement);
> String tgtStr = target.toString();
> - String replStr = replacement.toString();
> int j = indexOf(tgtStr);
> if (j < 0) {
> return this;
> }
> + String replStr = replacement.toString();
> int tgtLen = tgtStr.length();
> int tgtLen1 = Math.max(tgtLen, 1);
> int thisLen = length();
>
>
