Pull requests & Code reviews

Pull requests

Making a new branch

Before you start working on a new feature, you should make sure you're working on a suitable branch. To achieve this, do the following:

1. Switch to the master branch. Make sure you don't have any uncommitted changes that may be in conflict.

   $ git checkout master
   

2. Pull the latest changes.

   $ git pull
   

3. Create your branch and switch to it. Please follow the naming guidelines below.

   $ git checkout -b <branch-name>
   

When you have made and committed some changes, you can push them as follows:

$ git push --set-upstream origin <branch-name>

Note that a simple git push will be sufficient the second time around.

If multiple people are working on the remote branch, you will need to run

$ git pull --rebase

before pushing.

Naming branches

Branch names have to meet three requirements. They should -

1. clearly indicate ownership, (e.g. a specific project)
2. be brief, yet descriptive, and
3. be consistent.

To achieve this, you should stick to a very short name, using at most three words in general. Further, you should prefix the name with the project that is being contributed to:

• mgt - Management (Head of Software)
• mot - Motors
• nav - Navigation
• rnd - Research & Development
• sns - Sensors
• stm - State Machine
• qa - Quality Assurance
• tel - Telemetry

Opening a PR

When opening your pull request, make sure to give it a descriptive name and a useful description. The latter should either reference the related Issue or describe directly what the intended outcome of the changes is. However, you should also avoid being to verbose. For example, you don't need to describe which parts of the code you changed because that is obvious from looking at the changes.

For every PR, you should add the Head of Software, your PM and another Software team member as a reviewer.

2022 Note: If I (Franz) don't review your code within 24 hours, don't hesitate to ping me on Slack!

Code Review

Purpose

There are three main properties that code reviews aim to ensure for all changes that are being made:

1. Correctness (does the code do what its supposed to?)
2. Functionality (is this a reasonable solution for the problem?)
3. Maintainability (will other HYPED members be able to read and understand the new code?)

How to review code?

As a reviewer, here are the steps you should take before approving a PR:

1. Make sure the code compiles and all tests pass. (This is done by CI, so it's straight forward.)
2. Coding style
   • variables
     • Is it clear what every variable is used for?
     • Are there any unused variables/parameters?
     • Is every variables stylised correctly?
     • Maybe some thing should be written into a variable for readability?
   • types
     • Is the const keyword being used enough?
     • Is the auto keyword used sensibly?
   • grammatical correctness
   • comments
     • too many? redundant? (e.g. x += 2; // Increase x by two)
     • too few?
   • Anything else that makes the code hard to read; clever code isn't always good!
   • Are all includes required?
   • Is the using keyword used? If so, is there any good reason for it?
3. Programming errors
   • If the reviewer can't verify the correctness of the code by themselves, it's bad code.
   • Integration with other subsystems. Are any invariants violated?
   • Are compile time guarantees being made use of enough?
4. Testing
   • Have a sufficient number of tests been written?
   • If a bug was fixed, has a test been added to catch something similar in the future?
   • Tests should never include the same logic as the code itself.
5. Documentation
   • Does it exist?
   • Is it accessible?
   • Is it correct?
   • Judge on a case-by-case basis.

Don't hesitate to point things out!

Done reviewing?

If they don't respond to the requested changes, ping the author of the PR on Slack to let them know.

Further reading

• Palantir: Code Review Best Practices