RFR: 8313651: Add 'final' keyword to public property methods in controls [v7]

Nir Lisker nlisker at openjdk.org
Fri Sep 8 18:22:48 UTC 2023


On Fri, 8 Sep 2023 15:01:34 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> In the Control hierarchy, all property accessor methods must be declared `final`.
>> 
>> Added a test to check for missing `final` keyword and added the said keyword where required.
>
> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   only public

Looks good. Added some minor comments.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java line 99:

> 97: public class ControlPropertiesTest {
> 98: 
> 99:     private static final boolean FAIL_FAST = true;

Please add a short comment on what this is for. Like "If true, an exception will be thrown on encountering the first violating class, otherwise a list of all violating cases will be printed".

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java line 102:

> 100: 
> 101:     // list all current descendants of Control class.
> 102:     private Set<Class> allControlClasses() {

Please use `Class<?>` instead of raw types. In 2 other places below as well.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java line 103:

> 101:     // list all current descendants of Control class.
> 102:     private Set<Class> allControlClasses() {
> 103:         return Set.of(

Not that it matters much since the method is called once, but why is there a need for a method that returns the same constant set? The set can just be a constant: `private final Set<Class<?>> = Set.of(...);`

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java line 216:

> 214:             } else {
> 215:                 System.err.println(msg);
> 216:             }

No need for an `else` clause since `throw` will exit the scope of the method if it's reached.

-------------

PR Review: https://git.openjdk.org/jfx/pull/1213#pullrequestreview-1618032226
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1320200154
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1320199287
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1320196311
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1320197915


More information about the openjfx-dev mailing list