Coding Standards

The following standards will be applied to all new pull requests. They are not necessarily followed religiously in all existing code (there is still a great deal of tidying going on), so please don't assume that you can copy examples. This page is a shortened and simplified version of the guide used internally by Alfa.

Naming

In general, naming should be as follows:

Element type	Formatting guide	Part of speech	Example
Package	All lower case, single word	noun	org.alfasoftware.morf.jdbc
Class	Capitalize each word, capitalize first letter.	noun or noun phrase	SqlStatementExecutor
Interface	Capitalize each word, capitalize first letter.	verb/noun phrase or adjective	UpgradeStep, HasCleanup
Method	Capitalize each word, except first which is lower case. The get, is and set prefixes should be avoided for methods which are not property accessors/mutators. Adhere to the Single Responsibility Principle. If your method is hard to describe, it probably needs breaking up.	verb or verb/noun phrase	frobnicateSplines(), run()
Accessors, mutators	Property accessors/mutators ("getter" and "setter" methods) should use normal JavaBean conventions.	--	getFoo(), setFoo(...), isFooable()
Variables, fields	Capitalize each word, except first which is lower case.	noun (singular)	i, j, currentItem
Constants	All upper case, separate words with underscore.	noun or adjective/noun phrase	BUFFER_SIZE
Exceptions	As class syntax, with Exception suffix.	noun or adjective/noun phrase	InsufficientCapacityException

Formatting

All code should use UTF-8 and be saved with trailing spaces removed.

Tabs should be avoided. 2 spaces should be used for indents.

Opening braces should be on the same line as the declaration preceding them. Example:

public class Foo {
  public void bar() {
    int i = 3 +
            7 + 5;
    if (i == 15) {
      // do thing
    } else {
      // do other thing
    }
  }
}

Extra blank lines should be used as appropriate to separate logical sections of code.

Methods should be separated by two linefeeds, which aids readability when there is a lot of Javadoc (which there will be!).

Javadoc

Javadoc is essential for all publicly exposed methods and all classes.

For full guidelines, see Joshua Bloch - Effective Java - Second Edition - Item 44. It's good advice. A summary is here.

In addition, a suitable copyright message must be included on all class documentation:

* @author Copyright (c) Alfa Financial Software 2017

Comments

All classes must include the Apache 2.0 licence masthead prior to the package declaration:

/* Copyright 2017 Alfa Financial Software
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *    http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

A class may also contain general comments for developers changing the inner workings of the class that are not relevant to developers using it. These comments should go inside the class after the declaration and should not form part of the javadoc section. Such comments may include:

• Technical notes (but please consider adding something to the Wiki and link to it)
• Known bugs or problems (please, please raise an issue and link to it. The // TODO or // FIXME prefixes should be used so that it also appears as a Task on some IDEs).
• Notes on workarounds in use (again, should also be documented)
• Notes on unusual or complex implementations

Visibility

All class fields should be private.

All classes and methods should have the lowest possible visibility:

private > default > public

Related code should be grouped into packages to avoid internal entry points being made public and thus becoming a public API.

Anything that is made public will be evaluated as an API for long term-supportability and overall footprint. If you are just refactoring or bug fixing, you may wish to avoid API changes to speed things up.

We aim towards the smallest possible API footprint.

Exceptions

Squashing

In utility libraries such as Morf, there should rarely ever be a need to squash exceptions:

try {
  frobnicateSplines();
} catch (Exception e) {
  log.error("Failed frobnication", e)
}

Generally, exceptions should be allowed to propagate. However, as above, if you ever do squash an exception, at the very least log it.

Standard Exceptions

Use standard library exceptions where appropriate and where they make sense. Only define new exceptions if there is a need to differentiate them from the standard library ones or if there isn't a meaningful standard exception.

Checked Exceptions

Checked exceptions should usually not be included in method signatures unless handling of a specific exception must always be considered in calling the method and could be recoverable. This tends to be a far rarer occurrence than was perhaps expected. In other cases, if a checked exception is caught and should be propagated, wrap it in a RuntimeException. Guava's Throwables has some useful utilities for this.

This should apply to all categories of I/O exception (e.g. SqlException or IOException) or reflection exceptions (e.g. IllegalAccessException), which rarely represent anything recoverable at the low level.

There are a few cases worthy of special consideration, though:

SqlRecoverableException

Descendents usually represent transient database issues which may be retriable.

In our experience these are rarely safely retried without rolling back the current transaction. In this case, we usually let a wrapping RuntimeException propagate out of the transaction, then check the Exception.getCause() chain to determine if a white list of known-recoverable checked exceptions are involved anywhere in the stack. If so, the transaction can be retried.

This is usually preferable to forcing every method between the cause and the outside of the transaction to propagate checked exceptions, but some leeway may be given in some cases.

InterruptedException

If a thread may actually be interrupted for valid reasons, this needs careful consideration. There is a great deal of guidance out there on this subject, so valid arguments are welcome.

We tend towards resetting the interrupted flag and propagating a RuntimeException:

try {
  Thread.sleep(1000);
} catch (InterruptedException e) {
  Thread.currentThread().interrupt();
  throw new RuntimeException(e);
}

The interrupted flag can then be checked higher up the stack to determine if processing should be aborted.

Logging

Morf currently uses the Commons Logging API (although this may change to slf4J prior to 1.0.0) and uses Log4J for logging in tests.

Be judicious in the use of INFO level logging. Excessive logging is a performance drag.

If there is a cost in constructing the debug string, ensure that TRACE and DEBUG logging is wrapped appropriately:

if (log.isDebugEnabled()) log.debug(createComplexString(foo));

Use ERROR with care, only to report exceptions which are disrupting program flow but which are being otherwise suppressed. Only do this at the point where the exception is squashed. We have had issues in the past with the same exception being raised 2 or 3 times as it propagates up the stack.

Testing

We expect unit tests for all new changes. There is a fairly high level of coverage in Morf already, but the intention is to increase coverage still further. Any pull requests with low test coverage will not be accepted.

Unit tests are run as part of the Maven build. We also run FindBugs, PMD and Checkstyle with some custom rulesets. Make sure your code passes.

Unit tests should be written using JUnit and mocking performed using Mockito. Both dependencies are pulled in by Maven by default.

How to write "good" unit tests is outside the scope of this guide. Please ask if you would like some specific advice.

As Morf is so heavily geared around database access, many of the existing tests are database backed (by default, using an in-memory H2 database) and thus halfway between unit and integration tests (e.g. TestSqlStatements, AbstractTestSqlDialect). This is deliberate and has worked well up to now, but suggestions for improvements are always welcome.

Other

A collection of other rules we live by:

• The methods in your class should generally perform one task each.
• Any method which is not private should not trust the parameters that are passed in. Check reference parameters are not null before trying to call methods on them. Check other arguments for range or bounds if appropriate. Throw an appropriate exception if the parameters are invalid.
• Four is the largest number of parameters a method should take before you should consider using the Builder pattern, parameter objects or decomposing the functionality.
• Avoid null. Use Optional. Definitely never return null from a method.
• Avoid overuse of inheritance. Adhere to the is-a principle. Generally service/strategy code is better modelled with alternative patterns such as composition or chain of responsibility.
• Implement equals & hashCode on value types or anything likely to be used in a Collection. Implement them properly (see Bloch: Effective Java, chapter 3).
• Implement toString liberally, but it must be fast.
• Be careful when making a class final. It makes mocking much harder. Generally use final on "value type" classes only. This also applies to final methods.
• Avoid float and double for anything which requires a precise number (e.g. currency amounts or pretty much any number from a database field!). Floating point arithmetic is almost certainly not your friend. See BigDecimal.
• Use long instead of int for any integer value which could grow large.