Page Comparison

...

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:

// Checkstyle: CyclomaticComplexity|MethodLength OFF -- more readable not split up
public void process(final Reader in, final Writer out) throws IOException {
    // Checkstyle:CyclomaticComplexity|MethodLength ON

...

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.

...

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

...

Tests are written using TestNG.  All  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:

final List<Element> elements = ElementSupport.getChildElements(config, qName);
builder.addPropertyValue("whatever", SpringSupport.parseCustomElements(elements, parserContext);

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.

...

Google Guava and java-support Libraries

The Google Guava and java-support libraries 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:

 log.level("Data connector '{}': Blah ...", getId());

 log.level("Attribute definition '{}': Blah ...", getId());

Rather than:

 log.warn("StoredID '{}' : Blah ...", getId());

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

 log.level("{} Blah ...", getLogPrefix());