Code review basics
There are many cases where code reviews are useful. For example in security audits or when evaluating third party code. In this short post I will focus on reviewing code for Pull Requests (PR) specifically.
If done well, reviewing code can bring your team a lot of good stuff. Code quality goes up, less bugs are introduced and it features as a knowledge sharing tool within the team.
Creating Pull Requests
First off, some general suggestions about creating pull requests:
1 issue/story = 1 branch = 1 PR
It makes sense to develop your feature in a feature branch and use a PR to merge it back to your master. It keeps things organised.
Focus
Every change unrelated to the issue adds to the noise. A Pull Request is not a good place to find entire classes run through an automatic code formatter. Only focus on the relevant changes for the issue. If you want to apply the boy-scout-rule (always leave the campsite cleaner than you found it) do it in a separate PR.
Keep it small
As this tweet below suggests, small PR’s work best. So if a feature is too big, split it up.
Provide context
Give the reviewer some context. Describing the reason for the change, the approach you took and how the reviewer can test your work. For example setup up a new database table or run some scripts.
Receiving pull requests
Now for the other side:
Priority
Reviewing a PR from a colleague should be prioritised over regular work. As you wait, the risk of merge conflicts rises and it’s demotivating having to wait ages before your work is reviewed.
Be polite
You review code to make it better but it also has a social function within a team. Surely you can find something nice to say about the work of a teammate?
Respect each other’s time
There is no better way to waste time than endless discussions about variable names. Be critical but not every improvement you can think of is worth fixing. Code standards and naming conventions should fix most of these discussions.
Contents of the review
Now what should you actually be reviewing? This is probably a mixture of functional and non-functional requirements.
These requirements differ per company, team or even per project but they always contain multiple layers. If your project has a definition-of-done look there for inspiration.
I like to think of the layers in a kind of PyRamid form, loosely inspired by the Maslow pyramid. Here is ours:
I know, the Pyramid thing is a bit cheesy, but it might help you remember the layered perspective you need when reviewing.
Not all layers have to be addressed by the same person by the way. In my previous company we had an extra person reviewing security for bigger features. Having functional testing done by someone else also makes sense.
What does your pyramid look like? Please let me know in the comments!
