Remove redundant final modifier from private methods

[mches]

Identifies an opportunity to remove the redundant final modifier from private methods (i.e. failing test). Fixes the newly failing test. (TDD)

Note the final keyword is not redundant on a non-private static method.

Expands the test case for issue #846, increasing the confidence in merged PR #848 and future refactoring.

[mches]

Thanks for the thoughtful review. I'll get it cleaned up.

[blacelle]

Could you add a small comment in the code (1 line)and a secomment line with the url?

If you have a good link for the non static case it would also be helpful (as I was unsure overriding was also unavailable from another nested class).

Like https://stackoverflow.com/questions/34938540/override-private-method-in-java

[mches]

Could you add a small comment in the code (1 line)and a secomment line with the url?

Done

[mches]

Ready for another pass

[blacelle]

(CircleCI is now properly configured for forked-PRs. Nice.)

[mches]

What do you think of this simplification? It considers the modifier keyword first rather than last. Do you find it easier to reason about?

    @SuppressWarnings({ "PMD.CognitiveComplexity", "PMD.NPathComplexity" })
    @Override
    protected boolean processNotRecursively(NodeAndSymbolSolver<?> nodeAndSymbolSolver) {
        Node node = nodeAndSymbolSolver.getNode();
        if (!(node instanceof Modifier)) {
            return false;
        }
        var modifier = (Modifier) node;

        if (modifier.getParentNode().isEmpty()) {
            return false;
        }

        var parentNode = modifier.getParentNode().get();

        switch (modifier.getKeyword()) {
        case ABSTRACT:
            return isImplicitlyAbstract(parentNode) && removeModifier(modifier);
        case FINAL:
            return isImplicitlyFinal(parentNode) && removeModifier(modifier);
        case PUBLIC:
            return isImplicitlyPublic(parentNode) && removeModifier(modifier);
        case STATIC:
            return isImplicitlyStatic(parentNode) && removeModifier(modifier);
        default:
            return false;
        }
    }

    private boolean isImplicitlyAbstract(Node node) {
        return isInterfaceLike(node)
                || (node instanceof MethodDeclaration || node instanceof AnnotationMemberDeclaration)
                        && node.getParentNode().filter(this::isInterfaceLike).isPresent();
    }

    private boolean isImplicitlyFinal(Node node) {
        return node instanceof EnumDeclaration || node instanceof RecordDeclaration
                || node instanceof FieldDeclaration && node.getParentNode().filter(this::isInterfaceLike).isPresent()
                || node instanceof MethodDeclaration && ((MethodDeclaration) node).isPrivate();
    }

    private boolean isImplicitlyPublic(Node node) {
        return node.getParentNode().filter(this::isInterfaceLike).isPresent();
    }

    private boolean isImplicitlyStatic(Node node) {
        // interfaces and annotations are implicitly static
        return isInterfaceLike(node)
                // enums are implicitly static
                // https://stackoverflow.com/questions/23127926/static-enum-vs-non-static-enum
                || node instanceof EnumDeclaration
                // records are implicitly static
                // https://github.com/projectlombok/lombok/issues/3140
                || node instanceof RecordDeclaration
                || (node instanceof ClassOrInterfaceDeclaration || node instanceof FieldDeclaration)
                        && node.getParentNode().filter(this::isInterfaceLike).isPresent();
    }

[blacelle]

This looks definitely simpler to read. Could you make this in a separate PR once this one is merged?

[mches]

Certainly

[blacelle]

(@mches Do you confirm you can merge by yourself?)

[mches]

(@mches Do you confirm you can merge by yourself?)

I see all green checks that it's ready to merge, but I have no write access to merge it myself.

Only those with write access to this repository can merge pull requests.