Jump to: navigation, search

Difference between revisions of "SMILA/Discussions/Checkstyle"

m
 
(11 intermediate revisions by 6 users not shown)
Line 1: Line 1:
 
 
== SMILA Checkstyle configuration changes ==
 
== 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'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.
  
Although I'm not that familiar with checkstyle configuration, I tried to restrict my suggestions to those that should be possible, according to the [http://checkstyle.sourceforge.net Checkstyle 5.3 documentation].  
+
I tried to restrict my suggestions to those that should be possible, according to the [http://checkstyle.sourceforge.net 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.
 
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.
Line 14: Line 28:
  
 
a) exclude "private" methods from javadoc check.
 
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.
 
b) exclude "private" variables from javadoc check.
  
c) increase allowed method length (e.g. to "100").
+
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.
 
d) javadoc methods: allow documented exceptions that are not declared if they are a subclass of java.lang.RuntimeException.
Line 24: Line 42:
  
 
f) allow inline conditionals  
 
f) allow inline conditionals  
 
  
 
t) different checkstyle configuration for test bundles
 
t) different checkstyle configuration for test bundles
  
t1) exclude check for "magic numbers".
+
t1) remove check for "magic numbers".
  
 
t2) javadoc methods: allow missing param tags.
 
t2) javadoc methods: allow missing param tags.
Line 36: Line 53:
 
t4) javadoc methods: allow missing return tag.
 
t4) javadoc methods: allow missing return tag.
  
t5) exclude all variables from javadoc check.
+
t5) remove javadoc check for variables
  
 +
t6) have no checksstyle for test classes at all.<br/>
 +
TM: maybe a bit harsh but i think it still would be OK.<br/>
 +
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:'''
 
'''Votings:'''
Line 44: Line 66:
 
!
 
!
 
! Andreas Weber
 
! Andreas Weber
 +
! Thomas Menzel
 +
! Andreas Schank
 +
! Jürgen Schumacher
 +
! Igor Novakovic
 +
! Daniel Stucky
 
|-
 
|-
 
| a)
 
| a)
 
| +1  
 
| +1  
 +
| +1
 +
| -1
 +
| -1
 +
| -1
 +
| 0
 
|-
 
|-
 
| b)
 
| b)
 
| +1
 
| +1
 +
| +1
 +
| -1
 +
| -1
 +
| -1
 +
| 0
 
|-
 
|-
 
| c)
 
| c)
 +
| +1
 +
| +1
 +
| 0
 +
| -1
 +
| 0
 
| +1
 
| +1
 
|-
 
|-
 
| d)
 
| d)
 +
| +1
 +
| +1
 +
| +1
 +
| +1
 +
| +1
 
| +1
 
| +1
 
|-
 
|-
 
| e)
 
| e)
 +
| +1
 +
| +1
 +
| +1
 +
| +1
 +
| +1
 
| +1
 
| +1
 
|-
 
|-
 
| f)
 
| f)
 
| +1
 
| +1
 +
| ++1 ;)
 +
| +1
 +
| +1
 +
| +1
 +
| --1
 
|-
 
|-
 
| t)
 
| t)
 +
| +1
 +
| +1
 +
| +1
 +
| +1
 +
| +1
 
| +1
 
| +1
 
|-
 
|-
 
| t1)
 
| t1)
 +
| +1
 
| +1
 
| +1
 +
| +1
 +
| +1
 +
| 0
 +
| 0
 
|-
 
|-
 
| t2)
 
| t2)
 
| +1
 
| +1
 +
| +1
 +
| 0
 +
| +1
 +
| 0
 +
| 0
 
|-
 
|-
 
| t3)
 
| t3)
 
| +1
 
| +1
 +
| +1
 +
| +1
 +
| +1
 +
| 0
 +
| 0
 
|-
 
|-
 
| t4)
 
| t4)
 
| +1
 
| +1
 +
| +1
 +
| 0
 +
| +1
 +
| 0
 +
| 0
 
|-
 
|-
 
| t5)
 
| t5)
 
| +1
 
| +1
 +
| +1
 +
| 0
 +
| +1
 +
| 0
 +
| 0
 
|-
 
|-
 +
| t6)
 +
| -1
 +
| 0
 +
| -1
 +
| -1
 +
| -1
 +
| 0
 +
|-
 +
| t7)
 +
| +1
 +
| +1
 +
| 0
 +
| 0
 +
| 0
 +
| 0
 
|}
 
|}
 +
 +
[[Category:SMILA]]

Latest revision as of 06:06, 25 January 2012

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