New Code Style rules for DSpace "master" branch

[tdonohue]

PR is now considered ready for merger, though any additional comments are more than welcome.

I'd like to propose the following new Java Code Style Rules for DSpace master branch (these rules will not be backported to any previous branches):

1. 4-space indents for Java, and 2-space indents for XML. NO TABS ALLOWED.
2. K&R style braces required. Braces required on all blocks.
3. Do not use wildcard imports (e.g. import java.util.*). Duplicated or unused imports also not allowed.
4. Javadocs should exist for all public classes and methods. (Methods rule is unenforced at this time.) Keep it short and to the point
5. Maximum line length is 120 characters (except for long URLs, packages or imports)
6. No trailing spaces allowed (except in comments)
7. Tokens should be surrounded by whitespace (see http://checkstyle.sourceforge.net/config_whitespace.html#WhitespaceAround)
8. No empty catch blocks in try / catch (must at least include a comment)
9. Each source file must include our license header (validated separately by license-maven-plugin, see pom.xml)

These rules are both listed and enforced in the checkstyle.xml configuration applied to this PR's branch. More information on Checkstyle settings can be found at http://checkstyle.sourceforge.net/checks.html

This PR serves as public notification of this proposal, and I ask that all interested developers add your feedback into this PR (feel free to comment on individual line numbers in the checkstyle.xml file). Please let us know if you have objections to any proposed rule, or propose tweaks or additions to the set of rules. If you approve of these code style rules as-is, you are also welcome to 👍 this PR.

By default, the Checkstyle configuration is currently not executed (as the existing code has many violations to these new rules, see below). However, you can test them locally by running:

# First, you may wish to ensure SNAPSHOT versions of JARs are installed in your local maven cache
mvn install
# Then, you can run this in any single module's (e.g. dspace-api) directory
mvn -U checkstyle:check

Currently, based on the rules proposed above, here's the counts of violations in each DSpace module:

• dspace: 0 violations
• dspace-api: 41,619 violations
• dspace-oai: 775 violations
• dspace-rdf: 72 violations
• dspace-rest: 1,952 violations
• dspace-services: 1,115 violations
• dspace-solr: 26 violations
• dspace-spring-rest: 7,482 violations
• dspace-sword: 7,134 violations
• dspace-swordv2: 1,640 violations

As the number of violations are significant, this work is being performed on a shared, public branch (code-style-rules). That way we can collaborate on fixing the violations.

More information on this effort (including other sample Checkstyle configs from other projects) can be found at: https://wiki.duraspace.org/pages/viewpage.action?pageId=90967266

[abollini]

It is fine for me. I have tested the eclipse plugin and it works (add notes to the wiki page).

[tomdesair]

I'm OK with the current rule set and I've added some proposals for additional rules.

[tomdesair]

Related to visibility, I propose to also disallow direct class member access except for protected and final constant fields:

<module name="VisibilityModifier">
    <property name="protectedAllowed" value="true"/>
    <property name="allowPublicFinalFields" value="true"/>
    <property name="allowPublicImmutableFields" value="true"/>
</module>

http://checkstyle.sourceforge.net/config_design.html#VisibilityModifier

In addition, I would also prevent any redundant visibility modifiers to keep to code clean:
<module name="RedundantModifier"/> -> http://checkstyle.sourceforge.net/config_modifier.html#RedundantModifier

[abollini]

I want only note that on the current javadoc for a REST class we have a violation that cannot easily solved...
https://github.com/DSpace/DSpace/blob/master/dspace-spring-rest/src/main/java/org/dspace/app/rest/utils/MultipartFileSender.java#L31

due to the long URL...

[tdonohue]

Good point, @abollini. It looks like we could simply tweak this setting similar to how Google does, and add in an ignorePattern exception for long URLs:
https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml#L44

[tdonohue]

This has now been resolved, as I've added exceptions for long URLs, packages or imports.

[cwilper]

👍 on these rules...it's a good opportunity to do this, and these particular ones have the advantage of being well-accepted and understood by other java developers out there.

A couple other non-controversial ones you might consider adding, while you're at it:

• https://google.github.io/styleguide/javaguide.html#s6.1-override-annotation
• https://google.github.io/styleguide/javaguide.html#s6.2-caught-exceptions

[mwoodiupui]

I'm more concerned that we have a common style at all, than that we change it. In particular, I would be very wary of adding too many new rules at once. Which qualities of code on the screen are most important? Which contribute the most to readability and comprehension? why?

Left to myself, I tend to use Allman brace style (although I only just found that it has a name). This is probably due to being a fossil from the PL/I era, when blocks opened with BEGIN and closed with END. I'll have to train myself to rely more on NetBeans' block-level indicator. Just grumbling....

The two things I have long wished we would enforce are tab/space discipline, and class and method documentation. Sadly I don't think Checkstyle can help with documentation that doesn't actually tell one anything significant. And then there's the issue of the well-known argument: when a method takes a org.dspace.core.Context, we all know what it is but one must write something anyway.

One other thing that just occurred to me is that we ought to keep in mind a distinction between style rules and code-quality rules. Styling is for human readers. (Documentation rules are for human readers who are assisted by tooling that is sensitive to doc comments.)

Beyond the rules themselves, I think that we would do well to make it simple for contributors to adjust their several development environments to follow those rules, and especially make it simple to make adjustments when developing for DSpace separately from those for use with local code (which may be subject to different standards). We'll need a clear human-readable articulation of The Rules. We would greatly facilitate adoption of The Rules by providing comprehensive and maintained style sets for all of the IDEs we can support. (I will help with NetBeans.)

[terrywbrady]

These defaults for javadoc seem sensible to me. I am glad to see the flexibility around params/returns/throws.

[mwoodiupui]

I'm a bit unhappy with making @param optional, but I can live with it. Expect to see them added whenever I have to puzzle out a method's requirements.

[tdonohue]

@mwoodiupui: I think @param should be highly encouraged, yes. I was mostly hesitant about making @param required because I didn't want to slow this work down by forcing us to write a lot of code comments from scratch. I think future PR(s) could add in any missing @param comments and then flip the switch to make them required.

[tdonohue]

@mwoodiupui : Quick note regarding IDE support. We've begun drafting some notes on enabling Checkstyle rules in various IDEs here: https://wiki.duraspace.org/pages/viewpage.action?pageId=90967266#CodeStyleGuide(WIP)-IDESupport I haven't yet tested NetBeans' Checkstyle import plugin, but would welcome enhancing the notes there.

The overall goal is to use IDE-specific Checkstyle plugins, which would mean we'd be able to simply maintain our checkstyle definition, and ask everyone to import it into their IDE of choice. That should make IDE support a bit more seamless, assuming the IDE's Checkstyle plugin has no "gotchas".

[mwoodiupui]

I've now tried out the NetBeans plugin "Checkstyle Beans".

It partially integrates with the editor. Rule violations cause the line to be underlined in wiggly pale yellow, and a "tag" icon appears in the left margin. Hovering on the tag shows the rule's message. Plugin and built-in annotations rotate as a single list when you click on the arrow icon that means "multiple annotations this line."

Unlike the built-in rule engine, no hints are offered to the editor to "just fix it". Corrections are not noted until the file is saved. Hovering on the underlined text itself does nothing, instead of showing the rule message(s).

So: using IDE plugins with a common style file works for NetBeans, but will take some getting used to.

[mwoodiupui]

I'd like to see <module name="IllegalCatch"/> at some point, but we have a lot of them.

[tdonohue]

I've made some minor enhancements to the rules, based on above comments. I only pulled in suggestions which seem minor / non-controversial, as I'd like to avoid larger code refactors in this first cut, and instead concentrate mostly on obvious wins. Here's what I've added in 2fccaa4

• An exception to the 120char max line length for packages, imports and URLs (based on feedback from @abollini)
• Requirement that import statements be alphabetized and grouped (as suggested by @tomdesair)
• switch statement checks. Must have a default clause, and no fall throughs allowed (as suggested by @tomdesair)
• Require that each line of code only include one statement (as suggested by @tomdesair)
• Require that catch clauses (in a try/catch) not be empty (must at least include a comment). (as suggested by @cwilper)

Further feedback is welcome. Again, though, I'd encourage us to limit the number of initial rules here to only those that are non-controversial, quick wins (with the primary goals being to standardize our code alignment and braces). We can always follow up this initial code style with further enhancements.

[tdonohue]

Another minor update. Behind the scenes, I've been working on bulk changes to the dspace-spring-rest module today per these code style rules, and made a few changes based on that work:

• Disabled the requirement that all public methods need Javadocs. There are too many methods in our codebase (all modules) that don't meet this requirement. In order to move forward rapidly, we'll need to disable initially, until all methods have Javadocs written
• Removed the indentation exception for throws statements. While it's nice to have, it's seemingly very hard to configure an IDE (especially IntelliJ IDEA) to automate / police. At this time, it may not be worth the effort.

I've also been documenting how to do bulk code style changes in IDEA here: https://wiki.duraspace.org/pages/viewpage.action?pageId=90967266#CodeStyleGuide(WIP)-Fixingthecodebase

[tomdesair]

I think that the current settings are a good starting point to start validating our code with checkstyle.

[pnbecker]

Good work, thank you @tdonohue. 👍

[kshepherd]

+1 thanks Tim!

[tdonohue]

Closing as this was included in the newly rebased #1952.
So this exact code style has been merged to master via #1952.