Code Reviews
All about code reviews from the author to the reviewer
Code reviews are one of the activities you will spend the most time doing in your software engineering career.
Unfortunately, it is not something software engineers learn in schools or are specifically mentored about.
Below is a detailed guide on how to conduct a code review from both the perspective of the author and the reviewer.
Author Perspective
Keep your pull request small. This is the most important advice in this post. Large pull requests with a lot of changes are hard to review and are always punted for later. Once deployed, they are harder to debug, monitor and rollback. Similar to the one class one responsibility concept, aim to have your pull request also do just one change at a time.
Have a structured outline. Start with a clear and concise title. This is what shows up in the git log. In the description, it is important to cover the below sections: What?, Why? + Context + Ticket, How is it tested? and Reviewers.
Help the reviewer help you. Add screenshots if it is a UI or front end change, curl requests/responses if it is a backend change, and add line comments on code diffs when more context is needed. Show the reviewers that the changes have been well tested. This will give them more confidence to stamp and approve the PR
Avoid having the PR grow in size. A lot of time reviewers would take the opportunity to ask you for a refactoring while you are touching a certain area of the code. This is a legit comment. However, doing the refactoring in the same PR will make it bigger and doing more than one change at a time. This is to be avoided at all cost. One thing you can do instead is promise to do a fast follow in a new PR to address the refactoring comment and make sure to really do it later to build credibility in your organization.
Be open for feedback. It is possible for the most experienced software engineers to go in a blind spot or end up with a non optimal implementation. Having an open mindset and understanding the feedback provided in the code review will go a long way in improving the quality of the pull requests. Treat code reviews as gift rather than a burden.
Reviewer Perspective
Do not auto approve pull requests. If you do not have enough bandwidth to deep dive on the pull requests and spend time understanding it and reviewing, do not be a reviewer. Auto approving with a LGTM (Looks good to me) without a proper review is worse than not reviewing at all. On the opposite side on this, do not be too slow to code reviews, answer comments, etc… The code author is most probably blocked and waiting for your stamp.
Do not start reviewing unpolished pull requests. If you are going to spend time reviewing the PR, it should be ready and in a reviewable state. Make sure the PR is ready, small in size, tested and is passing the build before you start investing time in reviewing it.
Be respectful and friendly. Put your comments in forms of suggestions and questions. Instead of saying move this logic to a new class say looks like this class is doing a lot. Should this logic be in a new class? This keeps the review friendly and constructive.
Pay special attention to non reversible decisions. All code changes are important but pay special attention for things that cannot be reverted such as new endpoints, new dependencies, public methods, etc…
Stamp with comments when needed. If you do not have a strong opinion on a certain comment or is a minor comment (usually preambled with nit), write it and then approve/stamp the PR. This will leave it up to the author discretion to either address the comment or not.
So what should you look for as a reviewer?
Make sure the PR is readable. We can spend a whole chapter describing what is a readable code but the best definition I can come up with is a code that is glanceable and easily tells us what it does
Make sure the PR is small in size.
Make sure the PR is properly tested.
Make sure the PR is doing what is supposed to do.
Make sure encapsulations and abstractions are well designed. Are we exposing a property/method that we should not? Is a class or method doing a lot? On the opposite, is an abstraction/encapsulation unnecessary? Is the implementation overly complicated or engineered?
Code reviews are important for every software engineering and engineering organization. I hope you liked this guide. If you did, subscribe to this newsletter. I will be publishing similar posts on software engineering and tech startups.
Software Engineering from the Frontlines Course on Maven
If you liked this article, I will be teaching a “Software Engineering from the Frontlines” course on Maven where I will teach hard-learned lessons I acquired developing large-scale products at companies such as Uber, Airbnb, and Microsoft.
Areed that smaller changes are almost always a good thing. If a PR grows larger than I expected, I'll often break it down into several so it's easier to review.