On Mon, 19 Apr 2021 10:38:17 GMT, Patrick Concannon <pconcan...@openjdk.org> wrote:
>> Hi, >> >> Could someone please review my code for updating the code in the `java.time` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - 8263668: Update java.time to use instanceof pattern variable I generally concur with `forax` about avoiding the use of single letter variables. src/java.base/share/classes/java/time/Duration.java line 1435: > 1433: && this.seconds == other.seconds > 1434: && this.nanos == other.nanos; > 1435: } Perhaps rename the argument and use `otherDuration` as the refinement. Otherwise, an inconsistency across various classes will creep in where the more specific variable has a more general name. In this case, the argument type is Object, so the argument name `otherDuration` is not strictly true. src/java.base/share/classes/java/time/Instant.java line 1306: > 1304: && this.seconds == other.seconds > 1305: && this.nanos == other.nanos; > 1306: } Ditto, `otherInstance` is not strictly always an instant and it would be more consistent to swap the names. `(other instanceof Instant otherInstant)`. ------------- PR: https://git.openjdk.java.net/jdk/pull/3170
