Have questions? Stuck? Please check our FAQ for some common questions and answers.

You are viewing an old version of this page. View the current version.

Compare with Current View Page History

« Previous Version 12 Next »

To maintain a level of quality in the codebase, the project maintains a set of coding and testing guidelines.


Indentation and spaces

  • Four spaces, NOT a tab character, are used for regular indentation. For code broken into multiple lines, eight spaces are used indent lines after the first:

    if (obj instanceof DefaultDevice) {
    return Objects.equals(this.id, other.id) &&
            Objects.equals(this.type, other.type) &&
            Objects.equals(this.manufacturer, other.manufacturer);


  • If the line has to break at the dot separator ".", the dot symbol should be at the beginning of the next line:
     

    public void describeTo(Description description) {
    description.appendText("status '")
               .appendText(status.getDescription())
               .appendText("'");
}


  • Trailing whitespaces should be removed in both code and comments.

  • There should be a space between keywords (if, for, catch, etc.), parenthesis, and braces, as well as when casting:

    if (timer != null) {
    try {
        DeviceId did = (DeviceId) elementId;
        ...
    } catch (IOException e) {
...

  • There should be a space before the curly braces in method definitions. Arguments are not padded (neither in definition nor invocation):

    public Device getDevice(DeviceId deviceId) {
    return store.getDevice(deviceId);
}

The project uses Checkstyle  to enforce some of the formatting mentioned above. Violations will prevent the build from succeeding.

Line Length

Recommended line-length limit is 80 characters, but it is understood if lines occasionally spill over this limit for aesthetic reasons. Chronic trespasses of the 80 character line-length boundary may fail the review.

Exceeding the maximum line-length of 120 characters will fail the build.

Comments 

Javadocs are used to document the codebase itself. Interfaces are heavily documented with Javadocs so that implementing methods (annotated with @Override) inherit the commenting. 

Here are a few specific Javadoc conventions that come up in reviews frequently:

  • All public/protected/package entities must have javadoc comments
  • Descriptions of interfaces/classes/enums should start with a noun, e.g. "Representation of..." and avoid starting with "This class...." 
  • Descriptions of methods should start with a passive verb, e.g. "Returns...", "Adds..." and avoid starting with "This method..."
  • "@param name description" should be specified on the same line and wrap as needed from there; there should not be a line break between name and description
  • Unit tests classes should also have class javadoc comment; methods within do not need to have javadoc comments, although it is not discouraged.


/**
 * This is Javadoc commenting for classes and interfaces.
 * Javadoc descriptions are full sentences.
 */
public interface Foo {
 
    /**
     * This is the format for Javadocs for public methods
     * and those defined by interfaces.
     *
     * @param param functions that take arguments have this
     * @return methods that return a value should indicate this
     */
    boolean sampleMethod(Integer param);
} 
 

At the time of this writing, various formats are used within the code. 

/**
 * Implementing classes should have Javadocs as well, to emphasize 
 * their function.
 */
public class FooImpl implements Foo {
	
	/**
     * Important variables and structures should also be 
     * commented so that it is picked up by Javadocs.
     */
	private static final int SAMPLE = 5;

    @Override
    public boolean sampleMethod(int param) {
        // classic one-liner
        boolean val = false;
        /*
         * Multi-line comments within the code may use this 
         * convention.
         */
        if (param < SAMPLE) {
            // FIXME: multiple lines may be commented like this
            // for code that may be removed or changed
            // return true;  
            val = true;
        }
        return val;
    }	
} 

Interfaces and classes

Naming 

Interfaces should always be given first pick of clear names that convey their purpose.  For example, if there is an interface representing a network device, and a class implementing it, the interface should be given the name Device, and the class, something indicating that it implements the Device interface, e.g. DefaultDevice.

Class and interface names whose names are based on acronyms such as IP, PCEP, BGP, etc. should continue to follow the standard camel-casing convention, e.g. IpAddress, PcepService, BgpMessage. This maintains readability and avoids confusion between names of classes and names of constants. There are a few exceptions to this in ONOS code-base, mostly in legacy code; new code should adhere to the camel-casing convention, however.

Referencing

Wherever possible, references should be made to the interface, and not the implementing class. This includes method parameters. 

Nested (Inner) classes

A class that implements functions specific to a particular class (e.g. its event handlers or services that it exports) should be implemented as an inner private class within the class. Such classes have names beginning with Internal-, e,g InternalClusterEventListener. 

Objects and Methods

Data types

Wherever possible, use a rich data type over a primitive (e.g. MACAddress versus a long). This reduces ambiguity.

Immutable objects

  • Classes that are intended to be instantiated as immutable objects (e.g. DefaultDevice) should have class all variables declared final.

  • Collections (Sets, Maps, etc) returned by getters should be immutable, or a copy of the existing Collection.

    @Override
public Iterable<Device> getDevices() {
    return Collections.unmodifiableCollection(devices.values());
}

 
// or, if copying
@Override
public Set<DeviceId> getDevices(NodeId nodeId) {
    Set<DeviceId> ids = new HashSet<>();
    for (Map.Entry<DeviceId, NodeId> d : masterMap.entrySet()) {
        if (d.getValue().equals(nodeId)) {
            ids.add(d.getKey());
        }
    }
    return ids;
}

Concurrency

  • Avoid synchronized(this) and synchronized methods wherever possible.

  • Opt for thread-safe objects such as ConcurrentMap, and if using synchronized, apply it to the structure that must be locked:

    protected final Map<DeviceId, LinkDiscovery> discoverers = new HashMap<>();

@Override
public void event(MastershipEvent event) {
    ...
    synchronized (discoverers) {
        if (!discoverers.containsKey(deviceId)) {
            discoverers.put(deviceId, new LinkDiscovery(device,
                    packetSevice, masterService, providerService,
                    useBDDP));
        }
    }
}

    Additionally, some structures such as Hazelcast's IMap have per-key locks.

equals() and hashCode()

Any objects that are compared, or stored in any hash based data structure (e.g. HashSet, HashMap), should implement these methods. For objects that implement them, comparisons should use their equals() method, and not ==. An exception to this rule are Enums.

Naming

  • JSON fields should be in camel case:

    {
    "aliasIp": "10.1.2.3"
}

    and not "alias_ip".

Logging

The codebase uses SLF4J for logging. DO NOT use System.out.*. 

The logger should be private final, and associated with the particular class:

import static org.slf4j.LoggerFactory.getLogger;
import org.slf4j.Logger;
...
private final Logger log = getLogger(getClass());
...
    /**
     * Checks if all the reachable devices have a valid mastership role.
     */
    private void mastershipCheck() {
        log.debug("Checking mastership");
        for (Device device : getDevices()) {

When adding logs for debugging, rather than using info or warn, use the debug or trace verbosity level and set the log level in Karaf to inspect the debug messages.

Usage of Guava (More of a tools section topic)

ONOS leverages guava in several areas. Some prominent areas are:

  • Checking for null method inputs - using checkNotNull(): 

    import static com.google.common.base.Preconditions.checkNotNull;
...
    @Override
    public void removeDevice(DeviceId deviceId) {
        checkNotNull(deviceId, DEVICE_ID_NULL);
        DeviceEvent event = store.removeDevice(deviceId);
        if (event != null) {
            log.info("Device {} administratively removed", deviceId);
            post(event);
        }
    }

  • toString() - ToStringHelper():

    import static com.google.common.base.MoreObjects.toStringHelper;
...
    @Override
    public String toString() {
        return toStringHelper(this)
                .add("id", id)
                .add("vlan", vlan)
                .add("ipAddresses", ips)
                .toString();
    }
  • Data structures such as Lists, ImmutableSet, and HashMultiMap

  • Unit testing - EqualsTester():

    import com.google.common.testing.EqualsTester;
...
VlanId vlan1 = VlanId.vlanId((short) -1);
VlanId vlan2 = VlanId.vlanId((short) 100);
VlanId vlan3 = VlanId.vlanId((short) 100);
 
// first two equality groups contain equal objects, last differs
// from constituents of first two groups
new EqualsTester().addEqualityGroup(VlanId.vlanId(), vlan1)
        .addEqualityGroup(vlan2, vlan3)
        .addEqualityGroup(VlanId.vlanId((short) 10));

Imports

Import statements are expected to be organized by the IDE. It is understood that different IDEs use a slightly different scheme for organization. Settings for both Eclipse and IntelliJ IDEA are available to minimize (but not quite eliminate) these discrepancies.

Duplicate and unnecessary imports will result in build failure.

 

Home : Contributing to the ONOS Codebase


 

  • No labels