Skip to content

CodeStyle

David Hebbeker edited this page Mar 29, 2019 · 14 revisions

K-9 Code Style

We're currently using the code style as specified in settings.jar. In Android Studio go to File > Import settings… and select the file. It will create a new code style profile called "K-9 Mail". None of your global settings will be overwritten!

What follows is the most common issues (the settings.jar has the full configuration) that get picked up in review.

Java

Package Names

com.fsck.k9 is the top level package.

Sub-package names should be single ASCII English words or rarely two words, all lowercase, with no hyphen or other separator.

Imports

The order and style of imports should be:

import java.util....

import a.b.c

import b.c.d
import b.d.e

import com.fsck.k9

import static org.mockito.Mockito.*

Classes

Class Names are expected to be upper camel-case starting (e.g. BaseAccount).

We prefer treating acronyms as normal words in regard to capitalization (hence ImapAccount not IMAPAccount).

In general, despite the use of packages, classes should be unique across the project where possible.

Prefer extracting classes to separate files where reasonable. Longer classes are less easy to maintain and harder to test individual features.

Methods

Method names should be lower camel-case (e.g. performQuery).

Most method names start with a verb. Well named methods are preferable to short names.

Methods should be separated from each other by a single new line.

Variable Names

Variables should be lower camel-case. (e.g. accountName).

They do not need to indicate the variable type (e.g. not accountNameStr or similar).

They also should not be prefixed by m (e.g. not mAccountName). The codebase contains many examples of this for legacy reasons, but it is not good practice.

Constants

Constants are expected to be upper snake case (e.g. ACCOUNT_TYPE) but otherwise follow the rules for variables.

If / Else

The expected format of an if-else block is as follows:

if (conditionIsTrue) {
    executeCommand();
} else {
    handleAlternative();
}

Note the space between the if and the open-paren, the close-paren and the bracket and the else’s position on the same line.

Switch / Case

switch (property) {
    case "1": {
	break;
    }
    case "2":
    // fallthrough should be used rarely, and noted by a comment:

    // fallthrough
    default: {
	break;
    }
}

Comments

Single line comments should be used sparingly. Prefer splitting up methods into smaller helper methods than writing comments. Having to explain what ‘this bit of the method does’ is generally a sign the method is too long.

Multi-line comments should be used even more rarely and might indicate a refactor would be useful.

Good use cases for comments are for noting work-arounds for bugs in Android.

TODO / FIXME / etc

For legacy reasons, there exists a number of TODOs in the code.

In general these are probably tracked by issues.

They almost never represent ‘easy fixes’ - many of them would require fundamental reworks.

New code should not add TODOs and will be rejected if it does. If a change leaves scope for improvement a GitHub issue should track that instead.

Javadoc

We don’t publish the Javadoc currently. As such almost nothing needs Javadoc. Certainly classes only consumed by K-9 don’t need Javadoc.

At some point we may write proper Javadoc for Providers and some of the K-9 Mail Library. But this will be properly designed and planned before-hand.

Unless the GitHub issue specifically mentions writing Javadoc, don’t write it. And delete any stuff auto-generated by IDEs.

The primary issue with it is:

  • It typically doesn’t tell you much more than the method signature
  • It is easy for the documentation to actively mislead, especially following fixes and refactoring
  • There’s no clear target audience
  • It makes class files even longer.

XML

The resource structure is well defined (with the exception of the long strings.xml file).

Resource names are lower snake-case (e.g. some_text)

Lint / CheckStyle / FindBugs

We would like to move towards a situation where these tools are enabled and fail the build if they raise errors. However we are not there yet.

New code should not add to the existing counts substantially. If you write large amounts of functionality consider at least running these tools to ensure you aren’t adding more stuff to fix before these can be enabled.

Automated Testing

Because of the nature of Android it is impractical to test on every device. In addition, email servers have quirks. For this reason we rely on a growing suite of automated tests.

Passing the automated tests is mandatory for code to be merged.

For new functionality we would like to see automated tests covering the new behaviour (i.e. if an option is added, tested both with and without it).

Much of the code is however still not subject to automated testing for a variety of reasons. Efforts to improve this situation are appreciated.

Enforcement And Improvement

At the time of this writing (2017-03-23) the code style is not enforced and much of the code base doesn't conform to it, e.g. in many places fields are still prefixed with "m".

If your changes touch a lot of files please don't reformat all of the affected files to use the new code style as this will make it much harder to merge existing branches. You should, however, try to use the code style for new code.

If you decide to fix the code style of a file/method, please do so in a separate commit that doesn't contain any functional changes.

Note: It would be great if you could extract the code style rules from the settings file and put them here in human readable form.

Other notes on general code quality

  • Use meaningful names for fields, variables, methods and classes.
  • Try to keep methods small.
  • Try to keep classes small.
  • Avoid comments. If you need comments to explain something, chances are your code needs more meaningful names. If you're working around quirks/bugs in Android or a library, please do add a comment explaining why your work-around is necessary.
  • Do not use K9.app in new code!

Over the years many different people have worked on K-9 Mail. In many cases K-9 Mail's code base is not a good example on how to write clean code. You're welcome to improve this situation with pull requests!

Please don't be afraid to submit pull requests even if you believe you can't meet the code quality standards we aspire to. Github's comment system for pull requests is a good tool that allows us to work together on improving the pull request.

Line endings

We're trying to make sure all source and resource files consistently use LFs as line delimiters to avoid merge conflicts.

On Windows, the easiest way to handle this is to have git do it for you. By executing $ git config --global core.autocrlf true at any command prompt. This will cause git to checkout files using Windows-style line endings (CRLF), but all checkins will convert line endings to LF.

On Mac and Linux, you can use $ git config --global core.autocrlf input to make sure you only commit LF line endings.

You can’t perform that action at this time.