Redundant modifiers in Java code and checkstyle

[Boris Sazonov]

Hi, Java lovers!

The problem: Java allows redundant modifiers in interface declarations.

public interface Foo {
    public void Bar();
}

Interface method declarations in Java are implicitly public and abstract. So public modifier for Bar in this snippet is redundant.

Adding these redundant modifiers is permitted, but explicitly discouraged by Java Language Standard (http://docs.oracle.com/javase/specs/jls/se7/html/jls-9.html#jls-9.4):

It is permitted, but discouraged as a matter of style, to redundantly specify the public and/or abstract modifier for a method declared in an interface.

From what I've seen, Chromium codebase is not very consistent with these modifiers. Most of the interface declarations are written without this redundant modifiers, but a lot of code uses them. For example: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java?l=62. BTW, "static" modifier for AccountChangeEventChecker interface is redundant too.

The possible solution: checkstyle has a builtin check for redundant modifiers.

http://checkstyle.sourceforge.net/config_modifier.html#RedundantModifier

Checkstyle is already used for presubmit Java style checks, so enforcing a ban on redundant modifiers woulld be as easy as adding o couple of lines to https://cs.chromium.org/chromium/src/tools/android/checkstyle/chromium-style-5.0.xml.

I've checked all Java files in Chromium main repo and it looks there are around 700 redundant modifiers now.

Pros:

1. More consistency in code.

2. No more annoying warnings from IDEs.

Cons:

1. This check is triggered for the cases where redundant modifiers arguably increase readability:

public interface Foo {
    public static final int BAR = 0;
}

Every field declaration in interface is implicitly public, static and final, so all these modifiers are redundant.

Some feedback on this proposal contained requests to add this rule to clang-format style settings. It would be good to have clang-format auto-fix this, but I don't see anything similar in its style options. From this it looks like they would like to keep up with the style guides of major projects, but I think it would require adding this rule to the checkstyle config (and style guide) first.

Thanks in advance for your feedback.

[Tommy Nyquist]

The suggestion to remove them from the code makes sense to me. They are unnecessary and do not help readability (at least not when people get used to them not being there).

Regarding the implementation of this; I think it would be great if we did a first pass of removing all the current uses of the redundant modifiers before enabling it in checkstyle.

And preferably, I think we would want this to give our users a warning while you are fixing all the current places, so there isn't a flurry of new ones while you're working on this.

So basically; I think you want to do the following:

1) Add to styleguide.

2) Add as presubmit warning.

3) Remove all current redundant modifiers.

4) Move to the normal checkstyle.

At some point during this (maybe already after step 1?) you'd also start work on updating clang-format.

--

Tommy

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To post to this group, send email to ja...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAJJcB9_Rt-yGccjisqVi_Kw%3DQpoQezBZ5ze%3DLV8a6xP4vyLuPA%40mail.gmail.com.