New rule? Enforce redundent modifiers in interfaces

[Stephen Colebourne]

In Java 8, developers are allowed to have static and default methods on interfaces. In Java 9, developers can write private methods on interfaces. As part of this change, my Java 8 best practices talk recommends that all methods on interfaces should be declared "public static", "public abstract" or "public default" (plus "private" ones in Java 9). https://www.jfokus.se/jfokus17/preso/Java-SE-8-best-practices.pdf slide 75

The rule I want is that redundant modifiers are not allowed, except on interfaces where they are required (as above).

I'd like this to be enforced by Checkstyle, perhaps as part of RedundantModifier, perhaps as an entirely new rule. WDYT?

thanks
Stephen

[R Veach]

> "public abstract"

Why is `abstract` not redundant for interfaces in this scenario? All methods of an interface are abstract by default unless `static` or `default` is added, right?
Is this just so all the methods line up in the column?

[Stephen Colebourne]

In Java 9, the following combinations of modifiers are allowed on interfaces
https://bugs.openjdk.java.net/browse/JDK-8071453

"public static"
"public abstract"
"public default"
"private static"
"private"

"static" - same as "public static"
"abstract" - same as "public abstract"
"public" - same as "public abstract"
"" - same as "public abstract"
"default" - same as "public default"

Since Java 8, I've argued that the existing coding standards for
interfaces are now wrong.
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-April/015803.html
http://blog.joda.org/2016/09/private-methods-in-interfaces-in-java-9.html
https://www.jfokus.se/jfokus17/preso/Java-SE-8-best-practices.pdf slide 75

What should be clear from the above list is that the first set of
modfiers are consistent, and match modifiers on classes. The second
set of modifiers are inconsistent, and require the developer to
remember that an absence of a modifier means package-scoped on a
class, but public on an interface. At this point, the different
meanings for an absence of modifier are just a confusing anachronism
in Java. So, this is not about methods lining up in a column, this is
about consistency across classes and interfaces, now that interfaces
are much more like classes.

Given the above, it should be clear why I want checkstyle to validate
that the modifiers are present in full on interfaces. At my day job
and in our major project Strata - https://github.com/OpenGamma/Strata
- we use this coding standard for all interfaces. See this interface
for example which demonstrates the consistency of the cosing standard:
https://github.com/OpenGamma/Strata/blob/master/modules/basics/src/main/java/com/opengamma/strata/basics/date/DayCount.java#L29

I'm completely open to how the check is implemented in Checkstyle,
whether as part of RedundantModifier or a new check.

Stephen

> --
> You received this message because you are subscribed to a topic in the
> Google Groups "checkstyle" group.
> To unsubscribe from this topic, visit
> https://groups.google.com/d/topic/checkstyle/KYF-9EEzsbs/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to
> checkstyle+...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Roman Ivanov]

yes, for now this check http://checkstyle.sourceforge.net/config_modifier.html#RedundantModifier

can not distinguish methods from class or interface by means of tokens. 

So Check requires update inside.

User need to explain somehow to Check what rule he want to enforce: before java8, after java9, ....

I will think about design of it a bit later , I will share here link to github issue.
​

[Stephen Colebourne]

It may be a year later, but it would be great to make this happen. Any further thoughts?

(The rule I want is that redundant modifiers are not allowed, except on interfaces where they are required )

thanks

Stephen

https://groups.google.com/forum/#!topic/checkstyle/KYF-9EEzsbs

[R Veach]

Feel free to create an issue with all details of how validation should be done and it should be configurable through properties. Once approved, anyone can work on implementation.

IMO, `public` and `abstract`, alone or with other modifiers, should still be considered redundant.

[Roman Ivanov]

Disussion was moved to:

https://github.com/checkstyle/checkstyle/issues/5756

https://github.com/checkstyle/checkstyle/issues/5783

[Roman Ivanov]

Hi Stephen,

Looks like you care about style in your projects.

Can you share with us links to opensource projects that have checkstyle config, 

if build process is failing with Error on any checkstyle violation 

we can reuse such projects in our regression testing on each commit. 

Benefit for your project is that you will never have regression :) on new version of checkstyle :) .

thanks,

Roman Ivanov

--
You received this message because you are subscribed to the Google Groups "checkstyle" group.
To unsubscribe from this group and stop receiving emails from it, send an email to checkstyle+unsubscribe@googlegroups.com.

[Stephen Colebourne]

On 10 May 2018 at 23:08, Roman Ivanov <romani...@gmail.com> wrote:
> Looks like you care about style in your projects.
> Can you share with us links to opensource projects that have checkstyle
> config,
> if build process is failing with Error on any checkstyle violation
> we can reuse such projects in our regression testing on each commit.
> Benefit for your project is that you will never have regression :) on new
> version of checkstyle :) .

Projects include:

https://github.com/JodaOrg/joda-time
https://github.com/JodaOrg/joda-beans
https://github.com/JodaOrg/joda-convert
https://github.com/ThreeTen/threeten-extra
https://github.com/ThreeTen/threetenbp
https://github.com/OpenGamma/Strata (checkstyle xml in
https://github.com/OpenGamma/OG-Tools)

Not every rule is set to Error level, but some are. Hope that helps.

thanks
Stephen

[Roman Ivanov]

Hi ,

Strata project looks like most covered the most,  but I failed to fail build 

$ git diff

diff --git a/modules/product/src/test/java/com/opengamma/strata/product/swap/MockSwapLeg.java b/modules/product/src/test/java/com/opengamma/strata/product/swap/MockSwapLeg.java

index 14a88e9..33bd19d 100644

--- a/modules/product/src/test/java/com/opengamma/strata/product/swap/MockSwapLeg.java

+++ b/modules/product/src/test/java/com/opengamma/strata/product/swap/MockSwapLeg.java

@@ -134,7 +134,7 @@ public final class MockSwapLeg implements SwapLeg, ImmutableBean, Serializable {

   /**

    * The serialization version id.

    */

-  private static final long serialVersionUID = 1L;

+  private static final long serialVersionUID = 1l;

 

   /**

    * Returns a builder used to create an instance of the bean.

$ mvn install -DskipTests -Dforbiddenapis.skip=true

...

SUCCESS

can you help me to get error of build ?

thanks,

Roman Ivanov

[Stephen Colebourne]

You need the -Dstrict flag to switch checkstyle on.
Checkstyle is only run on the src/main/java folder, not src/test/java.
Hope that helps
Stephen