Note: This document is a work in progress.html
For Git commit messages we recommend following the OpenStack commit message recommendations.java
License and Copyright headers need to exist at the top of all code files. Examples of copyright headers for each language can be seen below.git
Note: In case you need multiple Copyright headers simply duplicate the Copyright line for additional copyrightsgithub
-== C/C++/Java ===web
/* * Copyright (c) 2016 <Company or Individual>. All rights reserved. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v1.0 which accompanies this distribution, * and is available at http://www.eclipse.org/legal/epl-v10.html */
############################################################################## # Copyright (c) 2016 <Company or Individual>. All rights reserved. # # This program and the accompanying materials are made available under the # terms of the Eclipse Public License v1.0 which accompanies this distribution, # and is available at http://www.eclipse.org/legal/epl-v10.html ##############################################################################
<!-- Copyright (c) 2016 <Company or Individual>. All rights reserved. This program and the accompanying materials are made available under the terms of the Eclipse Public License v1.0 which accompanies this distribution, and is available at http://www.eclipse.org/legal/epl-v10.html -->
In General we follow the Google Java Code Style Guide with a few exceptions. See: https://google.github.io/styleguide/javaguide.htmlapache
All OpenDaylight projects automatically run Checkstyle, as the maven-checkstyle-plugin is declared in the odlparent.session
This will only shows warnings in the build log and will not prevent your project from building if code violations are found.oracle
A project can "opt in" to fail for violations of the Checkstyle rules configured in odlparent by adding the following to their pom.xml:app
<build> <plugins> <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-checkstyle-plugin</artifactId> <configuration> <propertyExpansion>checkstyle.violationSeverity=error</propertyExpansion> </configuration> </plugin> </plugins> </build>
If really needed, projects can to override individual Checkstyle rules on a case-by-case basis by using a @SuppressWarnings annotation:less
@SuppressWarnings("checkstyle:methodparampad") public AbstractDataTreeListener (INetvirtSfcOF13Provider provider, Class<T> clazz) { }
The rule ID (e.g. 'checkstyle:methodparampad' above) is the name of the respective Checkstyle module; these IDs can be found e.g. in the git/odlparent/checkstyle/src/main/resources/odl_checks.xml configuration, or directly on the Checkstyle website from the http://checkstyle.sourceforge.net/checks.html list. For example, for the http://checkstyle.sourceforge.net/config_coding.html#EqualsHashCode rule you would put @SuppressWarnings("checkstyle:EqualsHashCode").
This @SuppressWarnings("checkstyle:...") should in practice be very very rarely needed. Please put a comment explaining why you need to suppress a Checkstyle warning into the code for other to understand, for example:
@Override @SuppressWarnings("checkstyle:EqualsHashCode") // In this particular case an equals without hashCode is OK because public boolean equals(Object obj) { // [explain!] (I'm a certified grown up and know what I'm doing.)
Please contact odlparent-dev@lists.opendaylight.org if you feel a Checkstyle rule is too strict in general and should be reviewed.
The Evolving Checkstyle page documents how to test changes to Checkstyle rules.
{@inheritDoc} JavaDoc
This JavaDoc is useless visual noise that hinders code readability, it is not required to put this, and has no impact, as JavaDoc does this by default:
/** * {@inheritDoc} */ @Override // (or on a constructor)
The only case where {@inheritDoc} is useful is when you actually have additional Java documentation. Default JavaDoc interprets as replace the parent's doc. If you truly want the full text of the parent to be copy/pasted by JavaDoc in addition to your additional one, then use:
/** * {@inheritDoc} * For this specific implementation ... */ @Override // (or on a constructor)
See also https://github.com/sevntu-checkstyle/sevntu.checkstyle/issues/467 & http://tornorbye.blogspot.ch/2005/02/inheriting-javadoc-comments.html
IllegalThrows
Instead of declaring "throws Exception", in almost all cases you should instead throw a custom existing or new ODL Exception. Instead of an unchecked exception (unchecked = extends RuntimeException; if you must for some technical reason, but should be rare, and avoided), it's recommended to use a custom module specific checked exception (checked = extends Exception); which can wrap a caught RuntimeException, if needed.
In order to avoid proliferation of many kinds of checked Exception subtypes for various particular nitty gritty things which could possibly go wrong, note that it in ODL is perfectly OK & recommended to have just ONE checked exception for a small given functional ODL module (subsystem), and throw that from all of it's API methods. This makes sense because a typical caller wouldn't want do anything different - what you are "bubbling up" is just that one of the operations which one module asked another ODL module to do couldn't be performed. This avoids having multiple throws for each exception in API methods, and having problems with extendibility due to having to add more exceptions to the "throws" declaration of API methods.
The exception for "throws Exception" may be a main() method where it's customary to let anything propagate to the CLI, or @Test testSomething() throws Exception where it's acceptable (Checkstyle does NOT flag this particular use of "throws Exception" in @Test methods).
IllegalCatch
The IllegalCatch violation should almost never be suppressed in regular "functional" code - normal code should only catch specific sub classes of the checked Exception, and never any generic and/or unchecked exceptions.
In the old pre-Java 7 days, some people used "catch (Exception e)" to "save typing" as a shorthand for having to catch a number of possibly thrown types of checked exceptions declared with "throws" by a method within the try block. Nowadays, since Java 7, using a multi-catch block is the right way to do this. In addition to being "nicer" to read because it's clearer, much more importantly than, a multi-catch does not also accidentally catch RuntimeException, as catch (Exception e) would. Catching RuntimeException such as NullPointerException & Co. is typically not what the developer who used "catch (Exception e)" as shorthand intended.
If a catch (Exception e) is used after a try { } block which does not call any methods declaring that they may throw checked exceptions with their throws clause (perhaps not anymore, after code was changed), then that catch may really have been intended to catch any possible RuntimeException instead? In that case, if there exceptionally really is a particular reason to want to "do something and recover from anything that could possibly go wrong, incl. NullPointerException, IndexOutOfBoundsException, IllegalArgumentException, IllegalStateException & Co.", then it is clearer to just catch (RuntimeException e) instead of catch (Exception e). Before doing this, double check that this truly is the intention of that code, by having a closer look at code called within the try, and see if that called code couldn't simply be made more robust.
Proliferation of catch (Exception or RuntimeException e) { LOG.error("It failed", e); } in regular "functional" code is a symptom of a missing abstraction in framework code; e.g. an Abstract interface implementation helper with correct default error handling, so that functional code does not have to repeat this over and over again. For example:
Sometimes developers also simply don't see that an existing framework API intends implementations to simply propagate their errors up to them. For example, for Exception handling in:
Here are some cases where catch(Excepion) is almost always wrong, and a respective @SuppressWarnings almost never acceptable:
Only in lower-level "Framework" kind of code, catch (Exception e) is sometimes justified / required, and thus @SuppressWarnings("checkstyle:IllegalCatch") acceptable.
System.out
The RegexpSingleLineJava "Line contains console output" and "Line contains printStackTrace" should NEVER be suppressed.
In src/main code, System.out.println has no place, ever (it should probably be a LOG.info; and System.err probably a LOG.error).
In src/main code related to OSGi/Karaf/Felix console output, use the respective correct API to obtain the console to write to; e.g. CommandSession getConsole() which returns the PrintStream to use instead of System.out. This isn't just "cleaner", but System.out is a bug - because console output from commands using System.out would not work e.g. over an SSH session into Karaf.
In src/test code, there should be no need to write things out - the point of a test is to assert something. During development of a test it's sometimes handy to print things to the console to see what's going on instead of using the debugger, but this should be removed in final code, for clarity, and non-verbose test execution. If you must, do you a Logger even in a test - just like in src/main code. This also makes it easier to later move code such as helper methods from test to production code.
Javadoc Paragraph: < p > tag should be preceded with an empty line
Checkstyle (rightfully) flags this kind of JavaDoc up as not ideal for readability:
/** * Utilities for... * <p>This...
and you can address this either like this:
/** * Utilities for... * * <p>This...
or like this:
/** * Utilities for... * <p/> * This...
Different ODL developers agree to disagree on which of the above is more readable.
Ordering based on modifiers. This is based on visibility and mutability:
public static final fields
static final fields
private static final fields
private final fields
private non-final fields
private volatile fields
private constructors
public constructors
static factory methods
public methods
static methods
private methods
The first group should be very strict, with the exception of FieldUpdaters, which should be private static final, but defined just above the volatile field they access. The reason for that is they are tied via a string literal name.
The second group is less clear-cut and depends on how instances are created -- there are times when juggling the order makes it easier to understand what is going on (e.g. co-locating a private static method with static factory method which uses it).
The third group is pretty much free-for-all. The goal is to group things so that people do not have scroll around to understand the code flow. Public methods are obviously entry-points, hence are mostly glanced over by users.
Overall this has worked really well so far because;
Note this list does not include non-private fields. The reason is that public fields should be forbidden, as should be any mutable non-private fields. The reason for that is they are a nightmare to navigate when attempting to reason about object lifecycle.
Same reasoning applies to inner class, keep them close to the methods which use them so that the class is easy to read. If the inner class needs to be understood before the methods that operate on it, place it before them, otherwise (especially if it's an implementation detail) after them. That's when an inner class is appropriate of course.
OpenDaylight project builds can automatically run FindBugs, as the findbugs-maven-plugin is declared in the odlparent. Contrary to Checkstyle, this is not enable by default. A project can "opt in" to fail for violations of the FindBugs rules configured in odlparent by adding the following to their pom.xml:
<build> <plugins> <plugin> <groupId>org.codehaus.mojo</groupId> <artifactId>findbugs-maven-plugin</artifactId> <configuration> <failOnError>true</failOnError> </configuration> </plugin> </plugins> </build>
TODO Document correct <dependency> to use for annotations (com.google.code.findbugs:jsr305 VS com.google.code.findbugs:annotations), and see if <scope>provided needs to be repeated in projects using it or if having it on odlparent, where it's not yet, will be sufficient)
PEP8 is the Python standard that should be followed when coding any Python code with the following exceptions.
To automate pep8 scanning we recommend using a tox configuration as follows:
tox.ini
[tox] envlist = pep8 #skipsdist = true # Command only available in tox 1.6.0 [testenv:pep8] deps = flake8 commands = flake8 [flake8] max-line-length = 120
Unfortunately the version of tox installed in the Jenkins build infra does not support the skipdist parameter which is necessary if your project does not have a setup.py file. A workaround is to create a minimal setup.py file as follows:
setup.py
# Workaround for tox missing parameter 'skipsdist=true' which was not # introduced until tox 1.6.0 import setuptools setuptools.setup(name='project-name')