Please don't file an issue to ask a question. You'll get faster results by using the resources below.
- We have a Gitter channel for technical questions about RSK
- We have a Telegram channel for general questions or discussion about RSK
These are mostly guidelines, not rules. Use your best judgment, and feel free to propose changes to this document in a pull request.
-
Do not open up a GitHub issue if the bug is a security vulnerability, and instead refer to our Bug Bounty Program.
-
Ensure the bug was not already reported by searching on GitHub under Issues.
-
If you're unable to find an open issue addressing the problem, open a new one. Be sure to include a title and clear description, as much relevant information as possible, and a code sample or an executable test case demonstrating the expected behavior that is not occurring.
- Separate your changes into multiple commits
- If your pull request gets too big, try to split it
- Each commit should at least compile, and ideally pass all unit tests
- Avoid merge commits, and always rebase your changes on top of
master - Follow the Java styleguide
Prefer readonly accessors and injection over setters
An object’s dependencies might be injected during construction or later on with setters. Construction injection is preferred over setters because the object can never be in an inconsistent state as long as it was properly injected.
Setters on the other hand leave the door open to NullPointerExceptions, prevent using private final fields, and make it harder to reason about the dependency graph.
public class BlockChainImpl {
private final BlockRecorder blockRecorder;
public BlockChainImpl(BlockRecorder blockRecorder, ...) {
this.blockRecorder = blockRecorder;
// ...
}
public ImportResult tryToConnect(Block block) {
// ...
blockRecorder.writeBlock(block);
}Prefer private final fields
private final fields guarantee they are only assigned once, at object construction time.
This ensures that the injected dependency will always be in a consistent state as long as the object was properly injected.
public class SyncProcessor implements SyncEventsHandler {
private final RskSystemProperties config;
private final Blockchain blockchain;Avoid @VisibleForTesting
Breaking a class encapsulation just for testing is a sign that we either shouldn’t be testing that, or that we should do it differently. Think about the following questions first:
- Is this just an implementation detail?
- Can you perform the same validations on an injected dependency? (e.g. a mock)
Prefer Optional<T> over null
Unlike with nulls, you’re forced to check for existence before using an Optional value.
public Optional<byte[]> getReturnDataBufferData(DataWord off, DataWord size) {
long endPosition = (long) off.intValueSafe() + size.intValueSafe();
if (endPosition > getReturnDataBufferSizeI()) {
return Optional.empty();
}Always annotate @Nullable methods
I call it my billion-dollar mistake. It was the invention of the
nullreference in 1965 - Tony Hoare.
NullPointerExceptions have been the source of pain in many Java projects.
Even though Optional is the modern Java pattern to deal with optional values, nulls are still around and are the right choice in some situations.
If you absolutely need to return null, always annotate the method with @Nullable.
Everywhere else, you should assume objects aren’t null.
@Nullable
public static Coin deserializeCoin(byte[] data) {
if (data == null || data.length == 0) {
return null;
}Check not null preconditions
A common pattern is to use Objects::requireNonNull on constructor parameters. Current rskj code doesn’t do this for historical reasons (e.g. test code passes nulls), but we should do it everywhere possible.
public PendingStateImpl(
Blockchain blockChain,
BlockStore blockStore,
RskSystemProperties config, ...) {
this.blockChain = Objects.requireNonNull(blockChain);
this.blockStore = Objects.requireNonNull(blockStore, "null is bad");
this.config = Objects.requireNonNull(config, () -> "don’t " + "use it");
...Unused code should be deleted
Unless there's a good reason to keep it around, in which case you should add comments explaining why.
This includes code with tests but no usages in production.
- All control structures should use curly braces
- A closing curly brace should be located at the beginning of a new line
- An opening curly brace should be located at the end of the last line
- A closing curly brace and the next
else,catchandfinallykeywords should be located on the same line - When a method or constructor declaration has many parameters, you should prefer starting a new line and indenting all parameters with 8 spaces
- When a method or constructor invocation has many parameters, you should prefer starting a new line and indenting all parameters with 8 spaces
For automatic formatting in IntelliJ IDEA, the following xml files can be placed in the .idea/codeStyles folder in the root folder of the project:
codeStyleConfig.xml:
<component name="ProjectCodeStyleConfiguration">
<state>
<option name="USE_PER_PROJECT_SETTINGS" value="true" />
</state>
</component>Project.xml:
<component name="ProjectCodeStyleConfiguration">
<code_scheme name="Project" version="173">
<codeStyleSettings language="JAVA">
<option name="ALIGN_MULTILINE_PARAMETERS_IN_CALLS" value="true" />
<option name="CALL_PARAMETERS_WRAP" value="5" />
<option name="CALL_PARAMETERS_LPAREN_ON_NEXT_LINE" value="true" />
<option name="CALL_PARAMETERS_RPAREN_ON_NEXT_LINE" value="true" />
<option name="METHOD_PARAMETERS_WRAP" value="5" />
<option name="METHOD_PARAMETERS_LPAREN_ON_NEXT_LINE" value="true" />
<option name="IF_BRACE_FORCE" value="3" />
<option name="DOWHILE_BRACE_FORCE" value="3" />
<option name="WHILE_BRACE_FORCE" value="3" />
<option name="FOR_BRACE_FORCE" value="3" />
</codeStyleSettings>
</code_scheme>
</component>