Have you implemented OpenMRS? Please participate in the Implementation Site Survey. If you already have, thank you!
OpenMRS tries to follow a common coding style in order to make our code base robust: more readable and more reliable (if the same code is created by three different developers, it should look and read very similar). Our style conventions are designed to promote consistency and good coding practices, while reducing common errors. If you feel that our coding styles could be improved (by changing, adding, or removing something), please bring your comments to the Developers Mailing List.
We use a combination of tools (checkstyle, PMD, FindBugs, ...) to write down our coding conventions in a set of rules. These rules are picked up by services (provided by our own infrastructure like SonarQube or cloud providers like codacy) so that any addition of new code (via a commit or pull request on Github) is checked for violations of these rules. Having the rules in simple config files together with the source code enable any developer to run the same checks locally.
The following tools are used to ease code review, help new developers get used to our coding style and maintain code quality.
We use checkstyle for static code analysis. checkstyle defines rules http://checkstyle.sourceforge.net/checks.html which when violated in the code will result in errors or warnings. The checkstyle rules we want our code to adhere to are defined in the openmrs-core repostitory's checkstyle.xml
You can run checkstyle locally using the maven checkstyle plugin with the following command
This will generate an html report at
(the path is relative to the root of the openmrs-core repository)
The report will show you all the violations for each java class with their severity, the name of the rule and the location (the jxr portion in the above command enables you to navigate to the location of the violation in the java class). If you want to know more about a specific rule go to http://checkstyle.sourceforge.net/checks.html
Install PMD locally and try
Adjust the version and location of the PMD you installed and refer to the PDM documentation to see the available command line options.
At the moment you cannot check the code against the FindBugs rules locally using maven as with checkstyle. We are working on it
We have set up integration with codacy so that our rules will be checked on every pull request. Whenever you make a pull request your code will be commented by codacy and if its clean the check will pass and if not show you what parts of your code violated which rules. You can see the current status of the openmrs-core at https://www.codacy.com/app/openmrs/openmrs-core/dashboard
SonarQube checks the openmrs-core code once a day. You can find the issues it found here
Read FindBugs to see where the rules come from.
We aim to bring all our rules (FindBugs, ESLint) into the openmrs-core repository and to let codacy use both set of rules to check the code. Codacy is currently configured to read our checkstyle and PMD rules from the repository to check Java code that goes into it.
FindBugs rules will not be checked by codacy since that is a feature only available to enterprise users. If you know another cloud provider that satisfies our needs and supports FindBugs please tell us!
The topic is discussed here:
Make sure you have followed the How-To Setup And Use Your IDE so that your code changes will automatically be formatted according to the OpenMRS style.
The following conventions are applied to code in openmrs-core but we encourage module authors to adopt them as well.
Include curly braces around if clauses, and loops even when there is a single statement inside, e.g instead of this
It should be this:
Avoid unnecessary else statements, normally you should be able to configure your IDE to catch this and warn you about it, e.g instead of this
it should be this:
Make sure you have followed the How-To Setup And Use Your ID which will prevent you from running in two the following issues.
Imports that are not used should be removed, remember that you introduce dependencies that are not needed!
If you are importing multiple classes from a package, list them all individually, like in the following code block, since this makes it clear exactly what imported classes are in use in any file:
Do not do the following:
Please check your IDE settings it is often the IDE which merges several imports into a wildcard import.
Since Java 6 String concatenation is implemented through the
StringBuffer) class and its
append method (source: String javadoc). Commonly String concatenation is more readable than
StringBuffer), because takes up less space.
It is means that in most cases we should use
For concatenation when logging, see How To Log.
if (s == null || s.equals("")) ...". Instead do "
if (StringUtils.isNotEmpty(s) ..."
Do not leave commented out code in the source files. Just remove it. Others will think this has been left there for a purpose and thus will probably not dare to delete it which will leave this commented out code hanging around forever. There are some exceptions of course, such as: "When I'm pretty sure it needs re-enabled at some point, and to make sure I don't forget why that is, I leave a clear TODO comment, e.g. "TODO this needs re-enabled when TRUNK-XXXX is fixed". Most IDEs are good at parsing out all TODO statements." Rowan Seymour
OpenMRS uses the logging facade https://www.slf4j.org/ which in turn delegates to the log4j as the logging implementation. If you want some background info on these libraries and their relation to Apache commons-logging and spring read this http://docs.spring.io/spring/docs/4.1.4.RELEASE/spring-framework-reference/htmlsingle/#overview-not-using-commons-logging
If you want to log you should rely on the slf4j. The following shows a typical usage borrowed from their API documentation
SLF4J 1.6.x is not supported more than two String arguments. In this case you can use
You might need to construct a message for logging and want to prevent this construction from happening if the debug log level is not enabled. You could then come up with a pattern like this
which you should not use even if you see it in existing code (if you see this clean it up).
Instead you should use parametrized messages:
Why? Read https://www.slf4j.org/faq.html#logging_performance for the answer.
System.out.println() in your code; instead log
ex.printStackTrace() in your code; instead log
Modifiers should follow the order suggested by the Java Language specification, sections 8.1.1, 8.3.1, 8.4.3 and 9.4:
We enforce this via Checkstyle rule ModifierOrder
Booleanclass; rather, use
defaultcase if only to
assert false, since future coding changes may introduce unexpected cases.
returnstatement to avoid accidentally falling through to the next case. In the rare case that you desire a fall through, the next case should be preceded with
/* fall through */to indicate your intention.
false; rather, simplify the boolean expression by removing the comparison and, if necessary, adding a leading exclamation mark (!) if you are testing for
All classes and interfaces should, at a minimum, have a Javadoc comment at the class level
Public methods should have a Javadoc comment
Code that performs any non-obvious task should be documented for clarification
Specify the return value of every method. Return values that should be documented are: null values, empty collections (whether an empty sets or empty list is always returned, etc)
Please note that the commenting style from most textbooks is only a good practice when the comments are intended for a student learning to program. It is not intended for professional programmers. Take a look at these best practices for commenting your code.
RunTimeException; rather, you should catch more specific exceptions. Catching these high level exceptions can mask errors.
RuntimeException; rather, you should throw more specific exceptions.
Create an intuitive hierarchy of exceptions within the package they belong, only creating new Exception classes for when the need to branch on different types of exception within code/webapp are needed (add as needed instead of making extra classes "just in case" for every possible variation).
and later add:
when we realize the webapp needs to distinguish duplicates from invalid identifier exceptions.
All new public methods and classes should have a @since annotation for the version when they were introduced. If the class is new, then it is enough to just put it at the class level instead of each method. Something like: @since 2.0
Example: instead of this: