formalize openmrs-core coding rules and style

Tags: #<Tag:0x00007fe2df191e68> #<Tag:0x00007fe2df191cb0>
#1

Hello friends of clean code :smile:

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.

What I learned: The SonarQube profile OpenMRS contains all rules which lead to the issues on SonarQube.

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.

  1. 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).

  2. 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)

  3. Add this knowledge to https://wiki.openmrs.org/display/docs/Coding+Conventions

Take SonarQube down or not?
#2

@teleivo am in full agreement to this! Thanks again for the awesome efforts to ensure that we get healthier code for improving peopleā€™s health! :smile:

1 Like
#3

I ran FindBugs yesterday from Eclipse and got a couple of scary bugs (possible NPE). I wasnā€™t able to see if they are real bugs or false positives.

Also, several of the CheckStyle rules can be enforced just by configuring the IDE.

#4

did you import the rules from sonarqubeā€™s openmrs profile or just use default settings?

#5

awesome!

I updated https://wiki.openmrs.org/display/docs/Coding+Conventions#CodingConventions-Checkstyle can you please check if the command also works for you to generate a local checkstyle html report where you can navigate to the issues in the classes

#6

Default settings. Do we have custom rules for OpenMRS ? It would be great.

#7

yes

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?

#8

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 :slight_smile:

#9

codacy is now configured to read the checkstyle.xml from the repository (for those with codacy admin access see here)

I am playing with the google checkstyle.xml integrated it with our existing checkstyle.xml.

You can try it out on this branch https://github.com/teleivo/openmrs-core/tree/google-checkstyle

And run

mvn clean compile jxr:aggregate checkstyle:checkstyle-aggregate

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.

Please tell me what you think :slight_smile:

#10

@teleivo how about setting what you think is best, and then we review? :slight_smile:

#11

Agreed. We have a ton of AvoidStaticImport violations, specially in the test classes.

#12

Our checkstyle is now updated and picked up by codacy :sunny: 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 :slight_smile:
2 Likes
#13

@teleivo please do not get tired of my saying to you that thanks a million! :smile: I particularly like the way you do those commits and immediately update the documentation! :slight_smile:

1 Like
#14

never tired of compliments :stuck_out_tongue_closed_eyes: thank you :crown: for your code reviews and help!!

#15
  • checkstyle (rules file checkstyle.xml)
  • PMD (rules file ruleset.xml)

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

1 Like
#16

Exciting progress @teleivo :smile: I see our codebase getting healthier and healthier for each day that passes with your efforts! :tropical_drink:

1 Like
#17

Thanks for doing this @teleivo!

I just make some comments in on alerts and the rules themselves in another thread:

#18

lets please discuss the rules here so its all in one place :slight_smile:

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.

What number would you see as acceptable?

#19

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.

e.g. the String class in OpenJDK 6 has 3K lines: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/lang/String.java

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.