Skip to content

Reviews

All code and documentation must be reviewed before it is merged. No one can merge their own changes without approval from others.

Purpose of Reviews

The purpose of performing reviews is ensuring:

  • High quality
  • Consistency
  • Clear intent (see below)

Concrete elements to go through:

  • Readable as defined by the reviewer (see below)
  • Security checks protects against unauthorized execution
  • Alignment with existing system and architecture
  • Usability
  • Corner cases and proper error handling
  • Doc Comments and Testable Properties are adequate and correct

Selecting reviewers for a merge request

Choose and assign primary reviewers based on the rules that are present in the CODEOWNERS file.

You can choose to add other reviewers if their feedback is important for the change.

The historic list of primary/secondary reviewers can be found on the responsibility page.

Rules for who reviews what

The tech lead of the team has the overall responsibility for all code in repositories maintained by their team. The default set-up is that the tech lead reviews all code and documentation produced in the team.

However, the tech lead also has authority to delegate some of the review duty to others.
If the delegation of review duty is standardized into rules, then the rules must be documented.

We use the CODEOWNERS file to list the primary reviewers for each repository. The default rule is that all primary reviewers in this file must approve a change. Any additional review rules that are in effect for the repository they must be documented in a comment in the CODEOWNERS file.

Handling vacations, leave etc.

When primary reviewers for repositories are not available the tech lead is responsible that all reviews are assigned to someone else.

Before the tech lead takes vacation or leave he/she must make arrangements to ensure that all responsibilities are handled during the leave.

Intent matters

Semantics are important for the reader. We have tools that check syntax, the programmer needs to communicate how code should be used.

Interfaces

Interfaces express a responsibility, either for the code itself or for the user of the code. Try to be as specific as possible when communicating intent for an interface. Favor Composition over inheritance.

A few guidelines:

  • Good naming: CurrentTimeSupplier is better than LongSupplier
  • Single responsibility: Make sure interfaces have a single purpose
  • Clear context and placement
    • Place interfaces prominently in the package structure - if external developers should know and use them
    • Keep interfaces package private - if not
    • Move into relevant class if they only are used for testing, even if it means that there will be multiple similar interfaces.

Records

Records are a convenient way to reduce lines of code, but they are not intended to replace any and all classes. When you write a record you communicate to the reader that the following code is a data carrier first and foremost.

The JEP 395 Records has the following summary: “Enhance the Java programming language with records, which are classes that act as transparent carriers for immutable data. Records can be thought of as nominal tuples.”

The Oracle documentation for records further explains (bold emphasis added): “Record classes, which are a special kind of class, help to model plain data aggregates with less ceremony than normal classes. [...] A record's fields are final because the class is intended to serve as a simple ‘data carrier’.”

Summary

Records are intended as transparent and simple carriers for immutable data, meaning they should have the following properties:

  • Data Carrier: It's purpose is to store and/or transport data between different parts of the program.
  • Transparent & Simple: It should represent the data in a straightforward and concise way, without adding any additional complexity or behavior beyond what is necessary for carrying the data.
  • Immutable: It cannot be modified after it is created. This includes any nested objects.

Readability

Code gets reviewed primarily in GitLab which does not support advanced highlighting like type hinting or fast documentation lookup. Code should be verbose enough so it can be read and understood in these environments. Humans are not compilers and should not be forced to be so.

Process

Comments in a review must be resolved by the creator of the MR when the appropriate changes have been made and uploaded.