SMILA/Discussions/Checkstyle

SMILA Checkstyle configuration changes

compromise

1. brox_checks.xml is replaced by two new checkstyle files: smila_checkstyle.xml, smila-test_checkstyle.xml (for test bundles)

2. Apply changes where everyone agreed: d, e, t, t1-t5, t7

3. Regarding a/b: Compromise to reduce javadoc overhead is, to not realize a/b but to apply t2-t4 to _all_ (private and public) methods, not only in tests. (BTW, this can not only be applied for private methods cause it seems that you can't have different checkstyle settings for private and public methods.)

4. Regarding c: Majority has decided ;) Maybe we can use these other complexity measures Jürgen mentioned instead (CyclomaticComplexity / NPathComplexity).

5. Regarding f: Majority has decided again. Daniel should be convinced with some Donuts... ;)

discussion

I'd like to suggest some changes for the checkstyle settings of our current checkstyle configuration file (brox_checks.xml) here. IMHO, the current configuration is too strict, produces much javadoc overhead and reduces developer acceptance.

I tried to restrict my suggestions to those that should be possible, according to the Checkstyle 5.3 documentation.

I'll label the suggestions with a,b,c,etc. so we can make a voting in the table below. Feel free to add some more suggestions.

Furthermore, I'd suggest a different checkstyle configuration for test bundles with some additional changes, see t1,t2,... below.

Suggestions:

a) exclude "private" methods from javadoc check.

(JS) IMHO it would be sufficient to not require to document every parameter, exception or return value explicitly. We could change this for all kind of methods: A good descriptive text plus good variable names should usually be sufficient as method documentation.

b) exclude "private" variables from javadoc check.

c) remove check for allowed method length (IMHO the methold length is just a hint for too much complexity in a method, but not a real proof - so this should be decided by the developer)

(JS) there are some other "complexity" metrics in checkstyle, maybe we can use these (additionally). On the other hand, the method length *IS* a useful metric: If a method gets too long, it will be hard to understand, because you are not able to get an overview about it. Next, such method tend to need (and have) a lot of inline comments, which make them even longer (Instead of commenting a part of a long method it's better to extract a method - the method name is a comment by itself!). So I think this warning *IS* valid and the developer should think about it. However, for test bundles this limit can be removed.

d) javadoc methods: allow documented exceptions that are not declared if they are a subclass of java.lang.RuntimeException.

e) javadoc methods: allow documented exceptions that are subclass of one of declared exception.

f) allow inline conditionals

t) different checkstyle configuration for test bundles

t1) remove check for "magic numbers".

t2) javadoc methods: allow missing param tags.

t3) javadoc methods: allow missing throws tag.

t4) javadoc methods: allow missing return tag.

t5) remove javadoc check for variables

t6) have no checksstyle for test classes at all.
TM: maybe a bit harsh but i think it still would be OK.
AW: I thought about that too, but finally I'd say that tests are too important to completely get rid of checks.

t7) allow '_' in method and class names (i use this to separate and group the test cases and make them more visible/readable, e.g. Solr_SearchPipelet_ScenarioA_Test, test_feature_variantA_OK, test_feature_variantA_Fail, ... )

Votings:

	Andreas Weber	Thomas Menzel	Andreas Schank	Jürgen Schumacher	Igor Novakovic	Daniel Stucky
a)	+1	+1	-1	-1	-1	0
b)	+1	+1	-1	-1	-1	0
c)	+1	+1	0	-1	0	+1
d)	+1	+1	+1	+1	+1	+1
e)	+1	+1	+1	+1	+1	+1
f)	+1	++1 ;)	+1	+1	+1	--1
t)	+1	+1	+1	+1	+1	+1
t1)	+1	+1	+1	+1	0	0
t2)	+1	+1	0	+1	0	0
t3)	+1	+1	+1	+1	0	0
t4)	+1	+1	0	+1	0	0
t5)	+1	+1	0	+1	0	0
t6)	-1	0	-1	-1	-1	0
t7)	+1	+1	0	0	0	0