• Task
  • Status: Closed
  • 2 Major
  • Resolution: Fixed
  • ehcache-core
  • cdennis
  • Reporter: amiller
  • October 08, 2009
  • 0
  • Watchers: 1
  • July 27, 2012
  • July 15, 2011


  1. Review rule changes in ehcache-core - change to exclusions or fix code as appropriate.
  2. Reconsider whether checkstyle audit should fail the build
  3. Review checkstyle rules for what makes sense to keep.


Alex Miller 2009-10-08

Rule changes to review per Greg:

 <module name="ClassFanOutComplexity">
     <property name="max" value="36"/>
 <module name="CyclomaticComplexity">
     <property name="severity" value="error"/>
     <property name="max" value="12"/>

Geert Bevin 2010-01-04

Annoyance: “Missing a header - not enough lines in file.”

On this file: package net.sf.ehcache.writebehind;

public interface WriteBehindCommitter { public void process(Object item); }

Alex Miller 2010-01-04

I find this one pretty annoying as I don’t really care for @author:

/NotificationScope.java:25: Type Javadoc comment is missing an @author tag.

Chris Dennis 2010-01-07

Doing a merge from trunk into a dev branch I hit:

/Users/cdennis/src/tim_checkouts/ehcache_stuff/ehcache/core/src/main/java/net/sf/ehcache/CacheManager.java:72:1: Class Fan-Out Complexity is 42 (max allowed is 40).

The stupid thing is I wasn’t going to refactor the class in my dev branch, so I just added it to the suppressions filter, and went on my merry way. Regardless of the result of the debate on how useful this rule is I just suppressed the problem because I wasn’t in the right position/place to be fixing it. In my opinion this kind of thing is a great argument for making these “violations” softer in terms of their enforcement.

Fiona OShea 2010-01-26

setup meeting to review issues

Chris Dennis 2010-03-05

Hit another annoyance today. The version of the maven checkstyle plugin we use (2.3) is outdated and as such will not allow me to use package-info.java files (the recommended way of doing package javadoc since 1.5). I tried upgrading the plugin version to 2.5 (the latest), but it seems various checkstyle modules have disappeared or changed names between the two versions so the configuration would need a complete update. Seems to me it would be a good idea to use the upgrading of the checkstyle plugin as an excuse to do a complete review of the checkstyle settings.

Chris Dennis 2010-03-05

Another annoyance:

private static final int MAXIMUM_CAPACITY = 1 « 30;

is not allowed since 30 is a “magic number”. All I required as a static constant for the highest power of 2 representable in an int. Much as I like the neatness of my eventual solution I still think the rule is ridiculous in this context:

private static final int MAXIMUM_CAPACITY = Integer.highestOneBit(Integer.MAX_VALUE);

Chris Dennis 2010-03-05

Another annoyance:

private int hash(int hash) {
    hash += (hash << 15 ^ 0xFFFFCD7D);
    hash ^= hash >>> 10;
    hash += (hash << 3);
    hash ^= hash >>> 6;
    hash += (hash << 2) + (hash << 14);
    return (hash ^ hash >>> 16);

is not allowed because all of these numbers are “magic”. Since I can only suppress rules at the file level (or using “magic” comment delimiters or hard line number ranges) I can’t really avoid having to suppress this rule for the entire file which kind of defeats the object.

Fiona OShea 2010-06-17

Lets set something up after Esperance and before Fremantle

Fiona OShea 2010-12-15

Seems like you are already doing this….

Hung Huynh 2010-12-15

checkstyle is being run for ehcache publishers

Chris Dennis 2010-12-16

This JIRA was primarily intended to prompt a review of the ehcache-core checkstyle rules. Unless I’m mistaken that hasn’t happened yet right?

Fiona OShea 2011-01-04

Chris can you schedule a meeting so that get this done. thanks

Fiona OShea 2011-02-22

Can we do this out of band? After we GA Freo?

Chris Dennis 2011-02-23

Yes, I’ll wait till everything has settled down after GA, and then schedule a meeting. I probably need to do some prep work to summarize the current rules (and manual supressions) before the meeting as well.

Chris Dennis 2011-03-17

I have created a Google Doc summarizing all the rules we currently have in place - I welcome any and all comments on any of these rules, both positive and negative: https://docs.google.com/document/d/1v6_jnUq54B8Abqt_TZGcXiwDOVqiURHFJOsQOyqJqKw/edit?hl=en&authkey=COuYneoM

Chris Dennis 2011-07-15

After an open review of the current checkstyle rules here are the changes I have made:

* *NewlineAtEndOfFile* removed (was warning) * *RegexpSingleLine* System.out.println expanded to System.(out|err).print* * *RegexpSingleLine* (?<!entry).getKey() and (?<!entry).getValue() added - but disabled due to number of false matches * *MissingDeprecated* removed (was warning) * *RedundantImport* removed (cases already covered by *UnusedImports*) * *ModifierOrder* elevated to error (was warning) * *RedundantModifier* removed (was warning) * *EmptyBlock* for LITERAL_CATCH added (but only at warning level) * *ArrayTrailingComma* removed * *ParameterAssignment* removed * *ExplicitInitialization* removed * *MultipleVariableDeclarations* removed * *IllegalInstantiation* added for all integral primitive wrapper types * *JUnitTestCase* removed (checkstyle is not run on test code) * *RedundantThrows* removed (was warning) * *HideUtilityClassConstructor* added (was warning) - currently disabled due to a bug * *NPathComplexity* custom threshold removed (reverted to checkstyle default of 200)

I’ve cleaned out the suppressions I could find that are no longer necessary. The empty catch block rule currently produces a number of warnings. I’ll be filing JIRAs to get these looked at. At least one of these which I have already fixed was a genuine bug.