These days code is a collaborative matter. Good developers are expected to both write great code and help others write code. Code reviews play a crucial part in this: they're the main tool a team has to ship great code and have fun while doing so.
😤 Reviews are hard
First of all, let me say it loud and clear: doing code reviews is damn hard. If somebody says giving a pull request review is easy it's probably because they never actually went over a pull request. Pull request reviews are hard because:
- you need to be polite and forgiving;
- it happens remotely 99% of the times;
- you need to think like someone else;
- everyone has their own coding style and it should be respected;
- code written in 10 hours is often reviewed in under 10 minutes;
- praise and criticism happens publicly;
- you are going to write stupid things and completely fail;
- ...
It also depends on the PR of course: certain PRs are pretty straightforward. You can relax with simple PRs, just approve them and merge them ASAP, your code deserves it.
I think you get the picture: pull request reviews are more about people and less about code and computers. In a productive development team reviews should happen often and be stress-free; they should help the team improve over time, spread knowledge and increase overall badassery.
📕 My Checklist
When I need to go over a pull request I do it several times and use each time a different point of view. I do this because checking everything at once is simply unbearable... I lose track of what I'm looking for and I need to start all over again. So each time I need to look at a PR I look for:
- syntactic correctness
- feature completeness
- meaningful use of git
✅ Syntactic Correctness
This is the first check: it's easy and fast (depending on the amount of code) and doesn't require a fresh brain and, usually, can even happen late during the day because it's just a matter of checking:
- do tests pass? Is the build green?
- does static code analysis report problems (i.e. [RuboCop][rubocop] and friends)?
- overall look of the code, are there any code smells?
- are the description and the commit messages written in proper English?
- does the PR respect all of the project conventions (i.e. branch naming, labels, ...)?
Generally, if something is clearly wrong I either assume the PR is not ready to be reviewed or straight away ask for a fix on those simple things before digging into the PR.
The key take-away here is automation: you'll save a ton of time by using tools like [overcommit][overcommit] or [Hound][hound], take the time to configure them properly so they can do the work you would do manually.
Another thing I do regularly is using [pull request templates][pr-templates] to add a checklist of what a project needs. Depending on the project and team, stating clearly the review process within the PR itself can be a huge time-saver, especially for simple requirements. There are a [ton of examples][pr-template-examples] you can choose from.
✅ Feature Completeness
This step is the hardest. First of all, I read once or twice the issue the PR is trying to solve (it should be linked in the PR description), then I go over the code changes, class by class, file by file and dig down the code as in depth as I can.
The idea here is to try to understand if the person that wrote the code:
- had a good understanding of the issue (asked the right questions, had the required skills, ...)
- wrote code that actually solved the issue
- didn't miss any edge-cases
- used the right design patterns (and didn't over-engineer the code)
All of the above can be extremely hard because you're generally looking at the result of several hours of work in a short amount of time (sometimes you even need that code in production and it won't help).
The key takeaway here is open-mindedness: never assume anything. Always be neutral when reviewing code, you should look at code as something that can advance a project, not like a form of art.
If you've been asked for a review there's a good chance you're the more senior developer (at least in that competence area). It can be tempting to request changes just because the code "looks ugly" or maybe because that doesn't look like something you would write... That's not the way you should do it: look for real problems, effective solutions and give developers the freedom to write the code in their style within the project guidelines.
✅ Meaningful Use of Git
So, the code looks solid, now onto the commits. Why are commits so important? I think of the git history as a ball of yarn that needs to be untangled: would you prefer the thread to be neatly rolled around... or would you prefer it to be randomly put together just for the sake of building a ball form?
This makes sense to me because we've always used [Trunk Based Development][trunk] so
our projects' master history is a single line of commits with each commit connecting
(hopefully) to the following one, in a single stream of changes. As the features grow and
evolve, the git history linearly represents what happened and why.
With commits following one another it's helpful to try to tell a story with [good git messages][git-messages] and overall common sense. I ask for changes on a PR when:
- meaningless history added (code is added and removed within the same PR)
- lots of changes and few commits (history is not explained properly)
- lots of commits and few changes (meaningless history probably added)
- commits explaining what the code does instead of saying why it does it
- merge commits (rebasing is a tidier way of making changes on PRs)
- commits not explaining what's happening (the infamous "fix the fix")
A lot of my time goes into reviewing git messages and it can sometimes feel like a chore (in particular for smaller projects) nonetheless understanding clearly what and why a particular piece of code is there is crucial for long term success of a team of developers. On long term projects developers often forget what happened a year ago, not to speak of new developers picking up the project later down the road.
💅 Tips and Tricks
And that's pretty much it! I'll end this post with a final list of random things that I find helpful:
- automate everything that can be automated
- giving reviews will help you write reviewer-friendly code, encourage everyone to review code
- speak to your team so that they know you don't hate them
- practice [Radical Candor][radical-candor]
- add gifs and funny pictures once in while to a review, a good laugh will set the right mood
- you're not on Twitter, don't troll around
- always explain why, never say "change this" without actually providing some examples
- if everything else fails, schedule a pairing session
- schedule your "review time" (I generally do 80% code/20% reviews)
