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

[Nir Lisker]

On Fri, 18 Aug 2023 22:17:15 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:
> 
>   review comments

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

> 82: import javafx.scene.control.cell.TextFieldTreeTableCell;
> 83: import org.junit.Assert;
> 84: import org.junit.Test;

These are JUnit 4, we should use 5 for new tests.

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

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

We use an empty line after the class declaration.

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

> 101:     // https://stackoverflow.com/questions/28678026/how-can-i-get-all-class-files-in-a-specific-package-in-java
> 102:     private Class[] allControlClasses() {
> 103:         return new Class[] {

I was also thinking that these should be generated by the contents of the package. When I looked at javafx.scene.control I saw classes that weren't controls, but were used by them, so they would need to be removed. So it's a question of whitelist vs. blacklist.

In any case, I'm not sure why you're using an array here. `Set.of` seems more suitable and will also check for duplicate entries.

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

> 230:     private String err(Method method, String message) {
> 231:         return method + " " + message;
> 232:     }

(This comment didn't appear in the review for some reason.)

I think that the code could be a bit more clean. I took a stab at it:


    Set<Class<?>> CLASSES = Set.of(/*...*/);

    private void check(Class<?> cls) {
        Map<String, Method> methods = Stream.of(cls.getMethods()).collect(Collectors.toMap(Method::getName, m -> m)); // the loop is also fine
        
        for (var method : methods.values()) {
            String name = method.getName();
            if (name.endsWith("Property")) {
                checkModifiers(method);

                String propName = name.substring(0, name.length() - "Property".length());
                check(methods, propName, "get", 0);
                check(methods, propName, "set", 1);
                check(methods, propName, "is", 0);
            }
        }
    }

    private void check(Map<String, Method> methods, String name, String prefix, int numArgs) {
        String methodName = new StringJoiner("", prefix, name.substring(0, 1).toUpperCase() + name.substring(1)).toString();

        Method m = methods.get(methodName);
        if (m != null && m.getParameterCount() == numArgs) {
            checkModifiers(m);
        }
    }

    private void checkModifiers(Method m) {
        int mod = m.getModifiers();
        if (Modifier.isPublic(mod) && !Modifier.isFinal(mod)) {
            if (FAIL_FAST) {
                throw new AssertionError(errorMessage(m));
            } else {
                System.err.println(errorMessage(m));
            }
        }
    }

    private String errorMessage(Method method) {
        return method + "is not final";
    }

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1299310658
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1299310698
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1299311517
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1299436111