Code Review: a skill
are you doing it right?
Yeah, you got it all wrong, Trust me! No matter how well you think you are at code reviews, it’s hard to be consistently good at it. For each ‘change request’ there is an author scratching his head on what you meant by that `xyz` comment. Either that, or you accepted the changes with no comments, and he/she is giggling that you were not able to catch that sloppiness in their code.
This series of articles stems primarily from 2 motivations:
- Consolidate my ~7 years of experience in reading, writing, reviewing, breaking and stitching numerous code blocks(mainly Go-Lang and JAVA).
- Seeing peers/juniors struggling to improve in this very basic skill which every software engineer should excel at.
Before I delve down into the nitty gritty of the agenda, I would like to cover few not so obvious topics which is fairly important even before the review begins. Once the code changes are done and your are ready to churn that CR/diff/PR(all three to be used synonymously henceforth) out, try and think about the following items:
Is my diff too big? or too small?
No diff is too small, the smaller the better. Now, that doesn’t means you go ahead and start raising single line diffs and beat the hell out of it. What I mean by ‘smaller the better’ is try to break your CR into a small logical block. A good rule of thumb is to limit the number of changes to 100 lines of code. Not including the unit tests.
There are many benefits to having small CRs, some of the major ones are:
- You get them reviewed quickly and get those comments early in the process, while you work on the remaining parts of the entire feature.
- Your reviewer is able to pay attention to small details rather than keeping track of files across a huge diff.
- Easy to catch bugs, imagine merging a 1000 line CR and then your world is on fire. It will be a lot slower to figure out the issue.Let alone the agony of your on-call peer.
- Wide range of peers can review multiple parts of the code for the feature depending on need.
Diff Metadata
No matter what tooling you have in-place for your CRs, it comes with a metadata section for you. And it is very important that you fill this section with as much details as possible. Some of they key areas to fill and the corresponding benefits are:
- Title: A format which I generally try to stick is:
[<project-name>][<diff-number>]<instructive-title>
Your CR title should be written in an instructive language. As if you were providing instruction for doing the same task. The diff-number is helpful when you are raising a number of diffs in a stack.
2. Reviewer: You will find a whole section below on how to select your reviewer for the diff. But never leave a diff without a reviewer.
3. Description: Under this section you can add details on how you came up with the change, reasoning behind it, may be link to the docs/tasks if you want to. The philosophy being ‘more the merrier’. It not only helps the reviewer but also adds a lot of context for you as an author when you revisit it at a later point in time through your git history.
Who should I add as a reviewer for this CR?
I like to choose two types of audience for the review(whenever possible). First, peers who can review the code structure, syntax and general language pitfalls. People are who are aware about the repository in general are generally a good fit for this. Second class of reviewers are those who can understand the business logic of the change and can provide valuable insights on the implementation details. They help us catch a nasty bug early in the process which you might have overlooked from that business requirement document(only if you get one from your PM, pun intended!) during the execution.
Generally we miss on adding the former type of reviewers and tend to loose on the valuable feedback which otherwise would have not only increased the quality of deliverable but also help you grow as a developer overall.
The above three are the most important things to keep in mind as you start on your journey to become a better developer by learning is this highly appreciated skill.
Follow up Articles (WIP)
- Dos and Don’ts: as an author
- Dos and Don’ts: as a reviewer
- NITs: nice to have changes
- Conflict Resolution in code review
- Death of a CR: ‘cry!!!!’
Although, I am taking a stab at creating resources personally. I feel there are also a bunch of good resources out there to help you with the same. Here are a few:
Coding Fun Fact: The first programmer was the daughter of a mad poet.
Request: Please add your valuable feedback in the comments. It would really help me improve the quality of content and bring it in-line with your expectations.
