Jump to: navigation, search

BestPractices/Developer Best Practices


Overview

The following wiki page will document the recommended best practices as recommended by the TSC committee.

This document is currently a work in progress and has not yet been ratified by the TSC committee. Please engage the mailing lists, Discussion Tab on this Page, and the TSC meeting to provide input on the below recommendations.

Recommended Best Practices

The following are the proposed recommended best practices for developers in all projects.

  • The intent is to find a balance between project autonomy, consistency in quality, and ability for new comers to quickly come up to speed and get involved.
  • Projects should be free to choose how they implement these recommendations. However the implementation a project chooses should be documented clearly for all developers to see.

Recommendation: Build Time Testing

Build time tests are tests which run as part of the building of your source code. Often referred to as "Unit tests" because most build time tests test small units of code or work, and are intended to run quickly.

Tests and linked pages on this Wiki provide further technical details to the principles outlined below.

Justification

Build time testing is intended to give developers (and everyone) a level of confidence in the code base to enable rapid development (including new features, bug fixes, refactoring etc).

Request

Ensure we have an adequate number build-time tests to enable developers to quickly and easily get notified of a regression in functionality do to an enhancement or refactoring of logic. We should strive to ensure all functionality is covered in such tests. Specifically, a build time tests should:

  1. Run at build time
  2. Run quickly
  3. Contain assertions to validate the functionality under test

Verification

It will be the responsibility of the individual projects to refine and define the implementation of the following recommendations. The committers on the respective projects will need to encourage and monitor commits

If code is the coin of our realm, tests are the gold in Fort Knox that give the currency its value.

Questions

Use this section to catalog outstanding questions / discussion points

  • [Chris Wright] And...defining functionality is, well, tough...Any thoughts on granularity? I think you're talking functional test vs. unit test.
  • [Devin]You are correct there are two distinct category of tests. My point here though was to make sure we that we provide tests for all of the functionality - not just 25 % or 30% of it. We want to make sure we cover all of the logic (otherwise why bother writing the logic at all?).
  • [Chris Wright] Can you clarify what you mean by "Contains assertions to validate the functionality under test"?
  • This was my attempt to state that simply calling a method from a test for example is not a high quality test. A high quality test should define assertions which actually validate the logic is working as desired (which of course means we have a well-defined list of "acceptance criteria" which tells you what the expected behavior is.
  • [David Bainbridge] I would love to see that each bug is associated with a test that demonstrates the bug so that we build up a library of regression tests.
  • [David Bainbridge] Have to admit I am a bit surprised that code coverage metrics and requirements are not part of the merge requirements, i.e. a build is considered failed unless tests cover x% of the code. I understand that this doesn't guarantee that the tests that are provided test the important code, even are good tests, but it does mean most code paths are run and something is better than nothing.
  • [Devin] This is where we start to run into the murky ground of project autonomy vs mandates - I would propose that we leave this up to projects to decide (though we can provide a set of possible recommendations).

Examples

Listed here are different possible implementations for build time testing that projects could consider using. The below are concrete examples of build time testing and the various accompanying technologies some projects have chosen to use. Recommendation of the TSC of this best practice does NOT mean you need to use these implementations.

  • Build Time Testing with JUnit - covers the basic JUnit testing framework along with some discussion about what makes a good test versus a bad test. Additionally we link to and discuss technologies that can aid you in creating high quality Unit tests quickly.

Recommendation: Comments

Commenting source code is intended to enable future readers/editors for the code to quickly understand and come up to speed on the logic in order to facilitate a community where anyone can read and modify any code.

Justification

Comments in code is intended to help readers of the code to quickly gain an understanding of the purpose of a file, class, method, etc. There is no way to avoid having to read code, but with a few well placed comments in classes you can quickly speed up developers understanding of the code.

Request

Ensure the following code is commented:

  1. Line level comments - any code which is complex or doing something out of the ordinary.
  2. File level comments - any file which has multiple purposes, or whose name doesn’t clearly state its sole purpose.

Verification

It will be the responsibility of the individual projects to refine and define the implementation of the above recommendations. The committers on the respective projects will need to encourage and monitor commits.

Questions

  • [Chris Wright] Where do you fit structured comments that are used to build docs?
  • [Devin Avery] It depends on the doc you are trying to build I guess. For example, if you want to build java docs for java files I think the comments could likely be one and the same. If you are talking about a different set of structured comments then I would need some more information. But I imagine any structured docs at the file level are relatively low level and are probably one and the same.
  • [Chris Wright] Related, but separate...external documentation (resurrecting the blueprint/design docs discussion ;)
  • [Devin Avery]Sounds like there is some history I am not privy too. I don¹t want to take this too far, but I do agree that functionality that is added should be documented with use-cases etc. If consumers don¹t know how to use it, what is the value?
  • [Colin Dixon] I think the best practice here should be to have each patch either point to a bug it's fixing or a feature it's providing. New/proposed features should be documented somewhere so that you can understand the context of a patch. We've proposed this in the past and I think it's what Chris is alluding to. It would be really awesome if we could then automatically produce a summary of patches for each project by bugs/features at the end of each week.

Access modifiers

TODO: There has to be authoritative and more complete external reference. Put a link here.

Recommendation

Class fields should be private. Accessors and mutators should be private, except when required to be public. In both cases simple accessors and mutators should be final.

Justification

Limiting visibility is good on multiple fronts. Hiding the classes makes it easier to discern what is public and what is an implementation detail.

For fields the situation is the same, except mutation comes into play.

If using "protected" modifier, invariants may change for subclasses, but at that point those fields should be moved to subclass and overall refactor of the base class is called for. Leaving fields protected means that you cannot make any assumptions about how they are accessed or mutated without having all the implementations. This has further implications if the state needs to be kept consistent -- tracking field mutation across a class hierarchy in face of concurrent access is a pure nightmare.

Therefore fields should always be private. A pure getter costs only source code, as JIT will aggressively inline accessor methods, leaving you with a well-defined API and equal performance.

Logging

Logging subsystem provides facilities for capturing, recording and reporting events which occur within the OpenDaylight system. These events are the primary means of system diagnostics and troubleshooting, serving a wide audience including automated monitoring systems, operators, administrators, support personnel and development engineers.

In order to provide a 'single system' experience, all software components should follow same basic rules for interfacing with the logging system. While it is not practical to force these rules on the various third parties, they should to be followed by all newly-developed components.

Please refer to the following wiki page for details on recommend logging practices - Logging_Best_Practices

Copyright

The following the requirements with regards to Copyright statements in all source code in the ODL projects.

New Files

  • Every file should contain a copyright statement, which at a minimum states the source code is available under the Eclipse Public License (EPL). By placing the copyright in all source files we make it easier for downstream consumers of ODL.
  • The OpenDaylight Project does not request or require copyright assignment to the OpenDaylight non-profit organization, so adding the original contributor's name or company is allowed.
  • We should treat a missing copyright statement as a bug so we can at least add the EPL license to make it easier for downstream consumers.

Example copyright:

/**
 * Copyright (c) <Date> <Company or Individual> and others.  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
 */

Important Notes:

  • If you are contributing to ODL on behave of a company or organization, please be sure to check with your company's legal department to verify if you need to use your company name's or if you can use your personal name as the copyright holder.
  • Be sure to place the current date and company name in the copyright. If copying a copyright please make sure that you place the correct company's name into the copyright (i.e. if you copy an existing copyright statement, make sure you change the name to give credit to the correct people or organizations!

Edited Files

If you, the editor, feel like you have made a significant contribution for which you want to issue a copyright, then modify the copyright, adding your company's name or individual name into the original header.

/**
 * Copyright (c) <Date> Existing Company One, and others.  All rights reserved.
 * Copyright (c) <Date> New Company Two 
 * 
 * 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 the example above, we added "New Company Two" into the copy right. "Existing Company One" already existed and remained in the copyright header. Whether or not you edit the existing copyright is up to you.

Important Notes:

  • Never remove or replace the copyright on a file. The original author still retains the copyright on the original work. You retain the copyright on the new edits only.
  • Significant is purposely left vague - you your best judgement. At the end of the day if the issue of who owns what changes ever arises it will be left to the lawyers and judges to determine the copyright ownership. Remember, all changes are logged in the git repository.

Links

OpenDaylight_Controller:Developing_With_Intellij - contains information about inserting copy right automatically with the Intellij IDE
https://www.eclipse.org/legal/copyrightandlicensenotice.php
https://lists.opendaylight.org/pipermail/discuss/2013-November/000926.html
https://lists.opendaylight.org/pipermail/discuss/2013-December/000927.html
https://wiki.opendaylight.org/view/Project_Proposals:Code_Prep_Suggestions

Build Before Submitting (Auto-sorting Pom.xml files)

Many pom.xml files are effectively self-modifying by a "sortpom" plugin declared in the common parent pom. ( controller project only at the time of this writing )

If a pom.xml is edited and pushed without having been built, it may be out of order and may be reordered during everyone's build, thus appearing as a modified file.

So be warned, only push pom.xml files after a successful mvn build! If you are reviewing pom.xml changes, please watch for this.