Last week James wrote a post about the challenges with scaling code reviews in the Java/Backend group here at Brandwatch. In this post I'd like to talk about code reviews themselves, specifically why we do them and how to create a high-quality Pull Request (PR) that facilitates a speedy and successful review.
Why do we even PR anyway?
Github's Pull Request feature is a joy to use. Developers work on forks of the main codebase, branching off the integration branch and opening a PR once they're ready for others to check out the changes they've made, before they are hopefully merged in. Github's UI makes examining the changes (diffs) super-easy (Pro Tip, append
w=1 to the PR's querystring to hide whitespace changes in diffs, making things even clearer) and gives everyone with access to the repository the ability to chime in. This helps us make sure that everything we ship is as well-crafted as we can make it.
Given that reviews can be so easily democratised with the Pull Request functionality, we quickly made peer code review a part of our development process. For both the contributor and reviewers, the PR process offers up useful opportunities:
The opportunity to learn from more experienced developers (and by experienced this could mean simply 'more used to this part of the codebase' as well as 'with superior knowledge' or perhaps even 'more battle scars')
The opportunity to teach - at Brandwatch we have a strong culture of helping each other skill up, so developers are encouraged to ask questions in a PR thread if there's a new concept or something they don't understand. This is a great opportunity for the contributor to share their knowledge, and also in some cases reflect on their decisions. Conversely, it's an opportunity for reviewers to constructively criticise decisions and point out alternatives and improvements.
How to structure a great PR
10 lines of code = 10 issues. 500 lines of code = "looks fine." Code reviews.— I Am Devloper (@iamdevloper) November 5, 2013
There's a serious underlying point made in the parody account's tweet above. Structuring a PR in a sensible way will reduce the cognitive load on the reviewers, and avoid the situation where an issue gets missed by an overwhelmed merger:
- Break your changes down into granular commits
- Order the commits in such a way that they 'tell a story'
- Provide comprehensive test coverage,
- Follow established conventions in a project
Let's examine each of these points in turn.
With the majority of new features added to a project (and most non-trivial bug fixes), there will be changes to the code that can be broken down into discrete conceptual and/or logical chunks. For example, a new chart in Brandwatch Analytics could well be comprised of:
- A model that collects data from the API
- A view that constructs the appropriate chart
- A view that sets up the user-selectable filters/options to manipulate the chart
Although the feature would be unusable without all the above parts, it makes sense to break the PR down into at least three discrete commits - one for each of the above parts and their associated tests. In practice, things are generally more complex, and a PR will be comprised of many small commits, but let's keep things simple for the purposes of illustration.
In a perfect world we'd be able to structure our thoughts and code things 100% satisfactorily from the outset, committing as we go, but it's fair to say that things don't often work out that way, so it's great that Git gives us some powerful tools to help us restructure our commits before submitting a PR. Tools such as
git add (with the
-p flag) and of course our old friend
git reset allow us to easily rewrite history and get our commits ship-shape before opening a PR.
A PR containing granular commits can be reviewed commit-by-commit, which makes the reviewers' lives much easier. Granular commits also significantly increase the utility of tools such as
git bisect and the horribly-named
git blame (why no
git praise?), so future development by others (and future you!) will be made easier too.
This is a simple tip that is heavily tied to the preceding concept. Aim to order the commits in a PR so that they build upon each other and form a narrative. This can be really helpful for the reviewer to get into your head and understand why you've coded a solution in a given way.
Comprehensive test coverage
In 2015 this should be a no-brainer, but it's worth restating. A solid suite of readable tests is essential to any project, and especially projects with multiple contributors. My personal preference is to use BDD-style assertions (we use Mocha and Jasmine at Brandwatch, normally with our own Steve Mason's excellent Expectations library) which make for supremely-readable tests.
some tests in Mocha, logically grouped using nested 'describes'
Following established conventions
In all our Frontend repositories at Brandwatch, we use the following tools to establish code style conventions
- JSHint / ESLint
Each repository contains config files for the above tools, and developers are asked to ensure that their editor/IDE is configured to adapt to the conventions set out in these config files. This helps eliminate style issues with PRs and lets reviewers concentrate on the meaty stuff.
Respect one another
Undeniably the most important factor in a good PR is the interaction between the submitter and reviewers. As a contributor, it's vital to approach criticism of the code you've submitted as constructive, and an opportunity to learn and further hone your skills. As a reviewer, it's important to bear in mind that the PR is the culmination of (often significant) effort on the part of the contributor, and to shape your critique in such a way that respects that effort. In short, to use the words of wiser men than me...
Be excellent to each other!