Introduction
At Abstract Leap, code review plays an important part of our development process. We use it both informally and formally in order to elevate the quality of the software that we produce. For all non-trivial pieces of development, we mandate that both peer code review and principal code reviews happen before any code is released.
This combination of code reviews has the following advantages:
- Spotting gaps - it's very difficult to objectively review a piece of work if you have done it yourself. Two heads are better than one!
- Knowledge transfer - a thorough code review will enable the reviewer to gain a good understanding of the new feature
- Quicker principal review - having a peer review first reduces the code review burden on principal engineers
While we make use of tools for performing our formal code review process we also encourage much more informal methods of code review, for example: whiteboard sessions, diagrams, mockups. When a particular feature has some complexity and uncertainty we use whatever mechanism is best for mitigating any associated risk as quickly as possible.
Our guiding principles
When performing code reviews we follow the following core principle (which is also Google's) and supporting principles:
In general, reviewers should favour approving a pull request once it is in a state where it definitely improves the overall code health of the system being worked on, even if the code isn’t perfect.
- There is no such thing as perfect
- Code reviews should be used for mentoring/knowledge improvement.
- Facts and data over opinions and preferences.
- Talk through conflicts when necessary (as opposed to using the pull request tool), and escalate if necessary
What do we look for?
While our code review process tries to uncover bugs and security issues prior to development we use it much more for ensuring the quality of the software as a whole stays high. To aid with that we focus on the following:
- Why is the change being made?
- Is the design good (does it match existing patterns, is it compatible with the architecture)?
- Is it functionally correct?
- Has anything been missed?
- Is it as simple as it can be?
- Does it break existing functionality?
- Have appropriate tests been written?
- Are things named well?
- Has the developer commented their code well?
- Are there any missed opportunities for re-use?
