I spent some time to understand where the rules from SonarQube came from and if there is any connection to the checkstyle.xml which is in the openmrs-core repository.
Here https://ci.openmrs.org/sonar/profiles to the right when you click on back up you can download an xml file with the rules. As you can see from the content of the file these are only FindBugs rules which are not in the github repository. Of 494 rules 165 are deprecated.
The checkstyle maven plugin was added back in 2010 (as you can find on github), the checkstyle.xml rules might be even older. The checkstyle.xml rules are not affecting SonarQube analysis as far I can see. They are not yet automatically synchronized with codacy (hope Iāll have that done soon).
Proposition:
I think we should formalize the code standard we want to adhere to. Lots of people are contributing and without automated checks in place the code looks like lots of different people wrote it. From my time as a newcomer I remember feeling confused by the inconsistencies in style (old, new unit test should styles; differences in indentations, ā¦). At the moment checkstyle rules are not applied and FindBugs rules are applied but not on PRs and in a place where contributors sending PRs are probably not looking.
These rules (FindBugs, Checkstyle) should all be in the github repository for anyone to see and run locally and for automated tools (like codacy) to pick them up.
I suggest we base our checkstyle.xml rules on the one which is available from Google which also comes with detailed explanations http://checkstyle.sourceforge.net/google_style.html (I would make a few exceptions for ex. they use spaces, we currently use tabs).
Lets extract the FindBugs xml rule file from SonarQube profile OpenMRS, remove the deprecated rules and put the file into the github repository. (codacy supports FindBugs only for enterprise customers, but I asked their support if it would be possible for OpenMRS; maybe we are lucky)
Here https://ci.openmrs.org/sonar/profiles to the right when you click on back up you can download an xml file with the rules. As you can see from the content of the file these are only FindBugs rules which are not in the github repository. Of 494 rules 165 are deprecated.
I am trying to clean them up and then put them into the github repo. I will share the file once I have done that so you and others can try them.
Unless someone with permission to edit the profile on sonarqube volunteers to clean the profile before I export?
sure, but the point is to provide a file that explicitly defines these rules such as with the formatter.xml. these checkstyle.xml and findbugs.xml can then be imported in the IDEs plugins for those tools which makes devs lifeās easier
It gives you a nice report at target/site/checkstyle-aggregate.html which also shows the occurences of violations per rule.
I would like others to chime in and decide with me on what rules we might not want and what severity we set for some. Like for example AvoidStarImport which IMHO should be an error and not a warning.
Our checkstyle is now updated and picked up by codacy
If you think a rule is missing or a rule is to strict, annoying, lets discuss it!
I updated https://wiki.openmrs.org/display/docs/Coding+Conventions which now contains detailed infos on how to configure Eclipse, IntelliJ (with pictures) since I feel it happens quite often that PRs on github that IDEs are misconfigured (star imports, formatting, import orderā¦)
Coming up:
I am sifting through the manual PMD rules that are configured in codacy and if there is a counterpart in checkstyle adding them to the checkstyle.xml so all rules end up in our repository
I am working on importing the findbugs rules into a clean xml into the repo with instructions on how devs can run it locally
I am working on an update to our README.md I think it needs a little more love to welcome and guide new people
@teleivo please do not get tired of my saying to you that thanks a million!
I particularly like the way you do those commits and immediately update the documentation!
are now in the openmrs-core repository and picked up by codacy.
FindBugs will follow soon its way into the repo (although as said this will not be used by codacy since we need to be an enterprise customer but one can run it locally and the SonarQube could also benefit from clean & up to date rules).
thanks @lluismf as you said the 3 tools are complementary. conventions (Checkstyle), bad practices (PMD) and potential bugs (FindBugs) as pointed out in this post
lets please discuss the rules here so its all in one place
you said:
I think methods of length 75 are already too long and I would hope that going forward we can get contributors to write smaller methods. So therefore I would suggest we keep the rule configured as is so new methods that are added are smaller. If PRs touch existing methods the reviewer can simply ignore this codacy comment and merge anyway. But this still helps in driving contributors to think about refactoring and write code that will be maintainable in the long run.
Also I checked on codacy filtering Checkstyle MethodLength and found ~60 methods over 75 lines which I think weāll manage to shrink over time. There are exceptional methods with > 400 which is simply horrifying.
Itās a matter of taste. At work the rules imposed by architecture (ivory tower guys) are 100 lines per method and 1000 lines per class. Preposterous, itās just a magic number picked up randomly.
First, lines is not a good metric, statements would be more accurate. Iām thinking about method chaining, wrapping ā¦
Second, even following SOLID principles some classes are going to be naturally big and splitting in several classes can be artificial and increase complexity.
HibernateConceptDAO has 2K lines and itās fine. It could be refactored with additional DAOs for the weak entities (concept attribute, concept datatype ā¦) but the result would be less cohesive.
ps: I agree with you that 400 lines methods are bad.
If you think a rule is missing or a rule is to strict, annoying, lets discuss it!
thank you 
for your code reviews and help!!