Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coding-conventions: Update to match our current practices #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions coding-conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ This article discusses various conventions and style rules that **bitcoinj** use

We use the standard coding style from Sun, but pay attention to the following rules:

Code is vertically dense, blank lines in methods are used sparingly. This is so more code can fit on screen at once.
Code is vertically dense, blank lines in methods are used sparingly. This is so more code can fit on screen at once. This rule does not apply to method-chaining (such as in `Stream<>` processing) where we generally follow the one-method-per-line convention.

We try to avoid lines going beyond 120 columns. However this is not a hard and fast rule. Content that is typically not very important like throws declarations are allowed to spill over if it results in more vertically dense code. Logic and comments should fit though.
We try to avoid lines going beyond 120 columns. However this is not a hard and fast rule. Content that is typically not very important like `throws` declarations are allowed to spill over if it results in more vertically dense code. Logic and comments should fit though.

Each file has a copyright notice at the top. You should put your own name there, unless you work for Google, in which case please put Google's name there instead. We do not mark classes with @author annotations, we have an AUTHORS file in the top level instead.
Each file has a copyright notice at the top. We have standardized on "Copyright by the original author or authors" for the copyright holder. We do not mark classes with @author annotations, we have an AUTHORS file in the top level instead.

## Comments

Expand All @@ -39,7 +39,7 @@ We like them as long as they add detail that is missing from the code. Comments
JavaDocs: all public methods, constants and classes should have JavaDocs. JavaDocs should:

* Explain what the method does **in words different to how the code describes it**
* Always have some text, annotation-only JavaDocs don't render well. Write "Returns a blah blah blah" rather than "@returns blah blah blah".
* Always include the one-line description of the documented item,annotation-only JavaDocs don't render well. For `public` methods always include the `@param`, `@return`, and any `@throws` tags.
* Illustrate with examples when you might want to use the method or class. Point the user at alternatives if this code is not always right.
* Make good use of {@link} annotations.

Expand All @@ -59,6 +59,7 @@ Good JavaDocs look like this:
* Returns the size of the current {@link BloomFilter} in bytes. Larger filters have
* lower false positive rates for the same number of inserted keys and thus lower privacy,
* but bandwidth usage is also correspondingly reduced.
* @return the size of the Bloom filter
*/
public int getBloomFilterSize() { ... }
{% endhighlight %}
Expand All @@ -75,25 +76,25 @@ We make use of volatile variables for references and booleans. For integers we u

We like IntelliJ Inspector. It can find similar bugs to FindBugs but with the advantage that it runs constantly in the background and seems to support a wider range of issues. We use annotations like `@GuardedBy` to help detect cases where variables aren't being properly locked, although the inspection for that isn't perfect by any means. We also use `@Override` and `@VisibleForTesting`.

We use nullity annotations. Field members and method parameters are assumed to not contain null by default, and cases where they can are marked with `@Nullable`. This allows static analysis engines like the Inspector to flag cases where we're possibly dereferencing a null pointer.
We use nullability annotations. Field members and method parameters are assumed to not contain `null` by default, and cases where they can are marked with `@Nullable`. This allows static analysis engines like the Inspector to flag cases where we're possibly dereferencing a null pointer.

## Assertions

We don't use the Java assert keyword, we use the Guava Preconditions class instead. The reason is that enabling assertions on Dalvik is a bit annoying and they are typically disabled in the field. However, we want assertions to always be enabled, even in production, because they are sometimes checking security sensitive matters.
We don't use the Java assert keyword, we use the `org.bitcoinj.base.internal.Preconditions` class instead. The reason is that enabling assertions on Dalvik is a bit annoying and they are typically disabled in the field. However, we want assertions to always be enabled, even in production, because they are sometimes checking security sensitive matters.

We statically import Preconditions whenever it's used, so we can write code like this:

{% highlight java %}
checkState(foo, "Object is not in the right state");
checkArgument(widget.isActive(), "This method requires an active widget");
Bar bar = checkNotNull(foo.bar);
Bar bar = Objects.requireNonNull(foo.bar);
{% endhighlight %}

You will see checkNotNull not only verifying arguments but used in other places too. This is typically done in order to prove to IntelliJ Inspector that a variable cannot be null at that point in time due to complex logic it can't figure out by itself (it knows how to read if statements in the same method and similar). This allows us to clear static analysis warnings that would otherwise flag a possible nullity violation.
You will see `Objects.checkNotNull` not only verifying arguments but used in other places too. This is typically done in order to prove to IntelliJ Inspector that a variable cannot be null at that point in time due to complex logic it can't figure out by itself (it knows how to read if statements in the same method and similar). This allows us to clear static analysis warnings that would otherwise flag a possible nullity violation.

## Java

`bitcoinj-core` targets Java 8 *and* **Android** 6.0 (Android API Level 23.) This is because we want to support a wide variety of Android phones. Due to our desire to make Bitcoin universally usable, we want to support developing countries where even new phones ship with old Android versions for cost reasons. Therefore, we abstain from using the latest Java language features and APIs in `bitcoinj-core`. Other components of **bitcoinj** generally target Java 11. (See the [release notes](https://bitcoinj.org/release-notes) for details and updates.)
`bitcoinj-core` targets Java 8 *and* **Android** 8.0 (Android API Level 26.) This is because we want to support a wide variety of Android phones. Due to our desire to make Bitcoin universally usable, we want to support developing countries where even new phones ship with old Android versions for cost reasons. Therefore, we abstain from using the latest Java language features and APIs in `bitcoinj-core`. Other components of **bitcoinj** generally target Java 11. (See the [release notes](https://bitcoinj.org/release-notes) and [pre-release-notes](https://bitcoinj.org/pre-release-notes) for details and updates.)

We make minimal use of fancy tricks like code synthesis or reflection. Thus we also avoid frameworks that rely on them too. One reason is to keep things simple and readable, another reason is to avoid closing doors to things like **GraalVM** native-image compilation, trans-compilation into other languages or aggressive dead code elimination.

Expand Down