Java Coding Conventions

Java Coding Conventions

The following coding conventions are used by all the Shibboleth Java projects. If you're looking to contribute large portions of code make sure you follow these conventions. Small patches (less than 50 lines) will be fixed up by the developer when they are applied.

Style and Format

The current Shibboleth developers use Eclipse for their IDE and all the Shibboleth projects contain the code template, code formatting rules, and checkstyle rules within the project themselves. So developers don't have to do anything special to set that up. Just remember to hit command/ctrl + f to format your Java files before committing them.

Developers are free to use something other than Eclipse but it is their responsibility to make sure code is formatted and styled appropriately as defined by these tooling config files:

• Code Templates

• Code Formatting Rules

• Checkstyle Rules

There are always circumstances in which making an exception to a style rule (particularly an automated one) is better than slavishly following it. In exceptional cases, developers can suppress particular Checkstyle rules for code sections of this kind. For example:

The example shows the most generally accepted case: disabling the CyclomaticComplexity and sometimes MethodLength checks in the case where a method is genuinely more readable when not split up just to satisfy the constraint. Be prepared to justify any suppressions you use, and if in doubt bring them to a developers call so that they can be discussed.  Always remember to re-enable any suppressed check as soon as you have passed the exceptional section, as shown above.

The general syntax of a suppression comment is:

• The word Checkstyle followed by a colon and optional white space,

• A list of Checkstyle checks to suppress or re-enable, separated by vertical bar ("pipe") characters,

• White space followed by the word OFF to suppress or ON to re-enable a test,

• Optional arbitrary text justifying the suppression.

Javadoc

All classes, generic types, fields, and methods must have Javadoc. Methods must document all arguments, returns, and thrown exceptions.

Do not use the following tags: @author, @deprecated, @exception, @serial, @serialData, @serialField, @version. Use of <code> is preferred to @code.

Use the @Deprecated annotation instead of the @deprecated Javadoc tag if you need to indicate that some part of the API is deprecated.

Naming Rules

• Use meaningful names

• Package names are all lowercase

• Constants are in UPPER CASE. Use '_' to separate words

• Class starts with an uppercase and each starting word is upper cased. No '_'.

• The class name of an exception class must end in 'Exception' or 'Error' depending on which it descends from.

• Methods, fields, and variables must start with lower case and then follow the same rule as classes.

• Acronyms should be uppercased when used as part of a name: for example, getURL() rather than getUrl().  This applies even when more than one acronym appears together, as in getLDAPURL(), but picking names so that this doesn't happen will make you more popular.  [Decided: 2013-05-31]

• Abbreviations are not acronyms, so the above rule does not apply to them.  For example, getId() is fine for "get the identifier".

• Never name a method parameter the same name as a Class field (i.e., if you ever have to use this you're doing something wrong)

Interfaces, Abstract, and Base Classes

Interfaces are used when defining a generic set of methods that needs to be implemented. Interfaces are not to be used to define behavior; the implementations of those methods should be allowed to vary as widely as is necessary for the particular concrete implementation. If your Javadoc for a method actually defines a contract for the method (e.g., under condition 1 you must do A, under condition 2 you must do B" etc.) you should be using a base class (see below).

In Shibboleth we distinguish between two types of classes which are abstract. An Abstract class is one which implements some, or all, of the methods of an interface(s) and thus defines a set of behavior that will be common to the concrete implementations of the interface(s) which extend this Abstract class. As a corollary, concrete classes which do not extend this Abstract class may have different behavioral implementations for those interface methods. The names of these types of classes must start with Abstract.

A Base class is an abstract class that does not implement an interface (with the exception of low-level language interfaces like java.io.Serializable or java.lang.Comparable) but instead directly defines a set of behaviors that apply to every concrete implementation of the class. Such behavioral methods are often, but not always, final methods. The names of these types of classes must start with Base.

And yes, it is unfortunate that one of the two types of abstract classes is Abstract. Over a million words in English and never just the one you need...

Scoping

All class fields which are not constants must be declared as private. If you want to allow subclasses access to it but don't have public getter/setter methods then provide protected ones.

If a class is part of the public API for the system you should avoid making methods private scoped unless you're absolutely certain no one should ever extend the class and override the method's behavior. Otherwise, mark the method as protected.

Use of 'final'

All method parameters must be marked as final. All method variables which are not explicitly meant to be reassigned must be marked as final. Utility classes (those containing only static methods) must be marked as final. Other classes and methods may be marked as final as appropriate.

We mark method arguments and variables final as a means of documentation. It allows the developer to say "this variable should never be reassigned". If a variable is not marked as such it indicates that the developer is explicitly treating that variable differently.

Tests

Tests are written using TestNG.  All classes should have associated unit tests and use of Cobertura to maximise code coverage is recommended.  Test classes should normally be named by appending "Test" to the class under test.

Parsing V2 configuration files

The following rules should be obeyed when writing the parsers for V2 configuration:

• If in doubt the V2 parsing code should be considered definitive. Cases where this is not the situation should be discussed.

• The parsing class should normally be named by appending "Parser" to the name of the bean being constructed.  This is in contrast to V2 where "BeanDefinitionParser" was appended.

• Where feasible, every element or type in a schema should have its own parser.  In certain cases this may not be feasible (for instance where a sub element has no corresponding bean associated).

• Most attribute lookup is namespace-unqualified (the usual calls are element.hasAttributeNS(null, "attributeText") or element.getAttributeNodeNS(null, "attributeText"))

• Defaults must always be wired into the parsers (we do not assume that schema validation will be running).

• Child elements should always be parsed in an order-insensitive manner.  This can be achieved by the following paradigm:

Null References

We've had a lot of bugs that result from an unintentional use of a null reference. This document explains the problems with null and its meaning within the language. In order to avoid this, avoid returning null unless null is a semantically meaningful value. For example, in StringSupport.trimOrNull(String), null means that the reference contains no useful content, for a particular definition of useful. In contrast, methods that return collections should in most cases not return nulls, but instead return an empty collection.

When accepting arguments in a method, the method should:

• Be null-tolerant in cases where the method is performing a simple lookup operation. For example, see the AttributeSupport#getXMLBase(Element) method.

• Fail-fast when receiving a null argument if the method is expected to mutate the state of the argument or current object state (i.e., this). As a hint, if you have a void method that isn't a setter it almost certainly falls in to this category (some setters do as well).

Finally, do use appropriate annotations indicating the nullability of a parameter or return value, the allowable content of collections, etc., as described below.

The use of the Guava Optional class is viewed as a less desirable choice at this stage for dealing with the null problem, but there may be individual cases where its use would be appropriate. Its use is subject to "strict scrutiny".

Annotations

There are some annotations that are used often enough to explicitly set some ground rules about. These annotations are documented below.

@Override

Located in package: java.lang
Use @Override liberally whenever you override a superclass method, or implement an interface method. (Developers meeting 2013-12-20)

@SuppressWarnings("unchecked")

Located in package: java.lang
This annotation must only be applied to a variable, never a method or class.

@NotThreadSafe, @ThreadSafe

Located in package: javax.annotation.concurrent

Class-level annotations that indicate that a given object may safely be used by multiple threads concurrently. The standard should be that a thread-safe object requires absolutely no thought about concurrence of any kind.

All classes should be documented with one or the other of these annotations, but in the absence of an annotation @NotThreadSafe should be assumed.

@Nullable, @Nonnull

Located in package: javax.annotation (but see note below)

Method and method-argument level annotations that indicate whether the return type or method argument may be nullable or not.

All non-primitive types must be documented with one of these constraints. When @Nonnull is applied to an argument this constraint should be checked (see below for information about constraint checking), but in any case a caller should assume that an unchecked exception will result from violating it.

In the absence of any documentation, parameters should be assumed to be @Nonnull and return values to be @Nullable.

NOTE: These annotations, while in the javax package namespace, are not a standard part of Java at this point. They were originally part of a JSR that is now dead, but are supplied by a jar file that places them in the originally proposed namespace.

@NullableElements, @NonnullElements

Located in package: net.shibboleth.utilities.java.support.annotation.constraint

Method and method-argument level annotations that indicate whether a collection or array, used as a return type or method argument, may contain null elements or not. When applied to a Map the constraint is over the keySet and the elementSet;

All collections and arrays used as input to, or output from, a method must carry one of these annotations. Consider using com.google.common.collect.Constraints in order to ensure that a returned, live collection maintains its NonnullElements constraint.

@NotLive, @Live

Located in package: net.shibboleth.utilities.java.support.annotation.constraint

Method and method-argument level annotations that indicate whether an object, used as a return type or method argument, could be mutated by another thread. That is, changes to the "live" object will be observed by another part of the system.

All collections and arrays used as input to, or output from, a method must carry one of these annotations.

@Unmodifiable

Located in package: net.shibboleth.utilities.java.support.annotation.constraint

Indicates whether a collection returned from a method is unmodifiable (i.e., it will throw an UnsupportedOperationException or something similar if any mutation operation is called). Note, this is not the same thing as a @NotLive collection as such a collection can be modified.

@NotEmpty

Located in package: net.shibboleth.utilities.java.support.annotation.constraint

Indicate that a given collection, array, or String can not be empty.

Constraint Documenting and Checking

The above annotations are, with the exception of @SuppressWarnings, @NotThreadSafe, and @ThreadSafe, about documenting certain constraints on either return types from or arguments to a method.

For method arguments, you should generally check @Nonnull constraints within the method using the net.shibboleth.utilities.java.support.logic.Constraint methods, which also allow for an assert-and-assign pattern.

Google Guava and java-support Libraries

The java-support library (Maven coordinates net.shibboleth.utilities:java-support:*) contain a large and growing collection of helpful classes. Review these libraries and use them as often as you can, particularly within methods. Exposing these within APIs requires more careful deliberation, but is not precluded.

New developers should spend at least a full day reviewing these libraries and checking out where they are currently used in order to get a feeling about where you might use them in your own code.

Use of Guava is generally less desirable at this point due to the gradual addition of many of its features into modern Java.

Log messages

Where possible messages should be start with the generic type of the component, then  the Identifier (if present), then the message hence:

Rather than:

Some components may have a getLogPrefix() method. In such cases, do: