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

Description

  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.

Comments

Alex Miller 2009-10-08

Rule changes to review per Greg:

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

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.