Skip to content

Pull Requests and Code Reviews

Remco edited this page Apr 21, 2022 · 12 revisions

Many of the the points described in this page are taken from an amazing article series written by Michael Lynch. We highly recommend giving these a read:

Pull requests

Pull requests help us write better code by providing a place to discuss code changes. Please help the reviewer as much as possible in understanding your changes.

Make sure you've considered the following, before requesting a review.

I've reviewed my own code

The best way to help your reviewer in understanding your changes is by standing in their shoes:

"Imagine reading the code for the first time." - Michael Lynch

It really helps to do this self-review on GitHub in the pull request view, to see exactly what the reviewer would. This also gives you a fresh look over your changes compared to the comfort of your local IDE.

I've written a clear changelist description

The PR description should “summarize any background knowledge the reader needs”. It provides context to the commits.

The commit messages should “explain what the change achieves, [..] and why you’re making this change”.

Consider this separation as different levels of abstraction. This helps the reviewer understand what changes you've made, why you've made them and how they fit in the bigger picture. It's also great documentation and stays close to the code in our version control.

"For a deeper dive into writing excellent changelist descriptions, see “How to Write a Git Commit Message” by Chris Beams and “My favourite Git commit” by David Thompson." - Michael Lynch

I've narrowly scoped my changes

"The smaller and simpler the change, the easier it is for the reviewer to keep all the context in their head" - Michael Lynch

  • Split up your changes in small commits
  • Split up many commits into multiple PRs
  • Inform your reviewer about your usage of small commits, to help them navigate

I've separated structural from behavioral changes

"Whitespace-only changes are easy to review. Two-line changes are easy to review. Two-line functional changes lost in a sea of whitespace changes are tedious and maddening." - Michael Lynch

Separating these different types of changes helps you narrow the scope of your changes. It also helps the reviewer by more clearly describing your intent for the changes.

Examples:

  • move/rename a file and make changes to it
  • move a code body and make changes to it
  • (1) test behavior, (2) refactor code, (3) change behavior

Try to split commits to help find functional changes:

  • test(x): verify that foo does x
  • refactor(x): move foo to bar
  • refactor(x): rename foo to baz
  • fix(x): make baz do y

Code review guidelines

Code reviews are necessary to end up with a high quality product, but they can be a medium for volatile debates, lead to frustration or even have negative emotional effects on everyone involved. We want to make code reviews a positive experience.

"Code reviews are an opportunity to share knowledge and make informed engineering decisions. [..] a good code reviewer not only finds bugs but provides conscientious feedback to help their teammates improve." - Michael Lynch

Please keep these guidelines in mind, when reviewing someone's changes.

Minimize lag

Please aim to perform a code review within one working day and try to minimize the time between rounds of review. The longer you wait to respond to comments, the more time others have to spend to get back into the topic.

Does the design fit the application?

Ask yourself if the changes fit in the design of the application. Do the changes belong in the application? Are they integrating well with the existing code?

Does it do what's intended?

We expect contributors to test their changes to verify it is working correctly. A reviewer should still think about any edge-cases. The contributor might've missed them and by staying aware during reviews we can prevent obvious bugs before they are merged.

Are the changes too complex?

A reviewer is a great judge for the complexity of code changes. The contributor might've been working on it for a while and knows exactly how the code works. This makes it harder to notice where the code might be too difficult for readers.
A reviewer should be able to quickly understand the changes.

Naming is a part of this complexity. Did the contributor use sensible names that are descriptive enough without being confusing. Feel free to ask for name changes even if you don't know an alternative yourself.

Tie notes to principles, not opinions

Explain both your suggested change and the reason for the change. In the best case, the reason should be based on principles (e.g. Don’t Repeat Yourself, Single Responsibility Principle). Try to stay objective to have a constructive discussion.

Aim to bring the code up a letter grade or two

Rather than overwhelming the author with too many comments, focus on scoping the change set into a smaller pull request. Changes can be postponed to another small pull request.

"Not perfect, but good enough" - Michael Lynch

Are the changes tested properly?

Make sure the changes are covered by unit/integration tests that verify all the acceptance criteria of the issue. But don't just check if there are tests. Also verify that they are sensible tests. Are they testing the correct things? Are the tests maintainable? Do you see anything that could result in flakiness?

Testing is not limited to just unit and integration tests. For some changes it might be preferred to do a QA run, or start a benchmark. This is something that needs to be judged based upon the changes. Don't be afraid to ask the contributor for these things.

Do the changes follow our style-guide?

Our style choices can be found in our Code-Style guide. We try to automate these rules where we can, but this is not always possible. A reviewer should be aware of this guide and reference it in reviews when contributors are not following it correctly. If a style argument is not yet covered pick a solution and raise the topic in the next team meeting.

Are the changes documented properly?

It's important we provide good documentation to the user. Reviewers should consider this during a review. Are there any changes in user facing interfaces, or does the way a user interacts with the system change, then these changes should be documented in the official documentation.

When a new feature is added we should make sure it is documented in the release announcement.

Finally, if there are changes in the way we validate BPMN / DMN the Camunda modeling team should be informed to adjust the linting rules.

Do we log the correct things?

To help our users and ourselves in case of bugs we have to make sure we have proper logging. A reviewer should mention it to the contributor if the logging is lacking, or if there is excessive logging being done. Please have a look at our logging guidelines for more information on logging.

Are the changes backwards compatible?

A reviewer should check if the changes contain breaking changes. If there are breaking changes the changes should be changed, or a discussion should occur on whether these are acceptable.

Should the changes be backported?

We support 2 minor versions of Zeebe. If the changes fix a bug they may need to be backported to the previous releases. A reviewer should remind the contributor to do this when applicable.