I was asked to put together a few guidelines for code review as a proposal for its use and reintroduction into our SDLC at my place of employment. Below are a few that I have found to work well. It should be noted, however, that code review is a part of team culture that means how it’s conducted and the rules that it follows very much rely on the team itself. If you have any ideas, comments, additional discussion, etc. feel free to reply.
Ground Rules:
There are literally hundreds of different ways teams conduct code reviews. Below are a few high level suggestions for our process.
Code review happens on branches (never post-merge in master)
Keep master in a demo-able/golden state at all times.
Keep master in a demo-able/golden state at all times.
Reviewers must be peers
You shouldn’t be reviewing your own code. Ever.
Author should decide on assignees but reviews are not limited to the assigned.
When authoring a pull request you should select at least one person who you believe has context on the features/fix you’re writing, however, everyone is encouraged to review and comment.
When authoring a pull request you should select at least one person who you believe has context on the features/fix you’re writing, however, everyone is encouraged to review and comment.
Reviews are not optionalThis simply means that nobody is exempt from code review. If you are writing code and you intend for it to be in production, it must be reviewed.
Reviewing comments must have value
Comments like “not going to work”, “needs more tests”, aren’t valuable to the author. If there’s a piece of code you disagree with, provide comments that suggest a solution.
Review should not be intrusive
Carve out *maybe* a half hour of your day (sporadically or at the end) to review code. You shouldn’t be rewriting the code for them, you should be attempting to follow the flow and understand their logic whilst applying your knowledge of the application as a whole.
Carve out *maybe* a half hour of your day (sporadically or at the end) to review code. You shouldn’t be rewriting the code for them, you should be attempting to follow the flow and understand their logic whilst applying your knowledge of the application as a whole.
Goals:
When viewing a pull request and conducting code review there should be a few goals you seek out. The following are a handful I am fond of:
Familiarize yourself with the changeset
The key here is to understand what’s being changed and how it might affect the application. This goes a long way in identifying quick solutions to bugs that occur post-deploy, in production.
The key here is to understand what’s being changed and how it might affect the application. This goes a long way in identifying quick solutions to bugs that occur post-deploy, in production.
Check style and coding standards
Ensure that your style matches the surrounding code and the coding standards we’d like to have going forward.
Collective knowledge and collective understanding of high risk areas
Because of how tightly coupled our codebase is (unintentionally in a lot of places), we should be de-siloing information about high risk areas of the code. Everyone should know when a there’s increased risk to a given deploy.
Catching bugs? Usually not. But if you do…awesome!
I’ve heard this summed up as:
Finding bugs in code review is like finding $5 on the ground because you went outside.
You wouldn’t expect to find $5 every time you went outside, but it’s a nice side-effect.
Identifying complexity
We should strive to identify changesets that are high risk or that need to be broken up. Furthermore, we should be identifying pieces of code that are overly complex within the given context. Readability over cleverness.
Code Review Outcomes:
The following are rewards that I believe we get for free by committing ourselves to the code review/pull request process.
Psychological effect of presenting your code to the team
There’s a subconscious side-effect of putting your code out for the entire team to review…It makes you double and triple check the code you’re submitting. We should take advantage of this!
Great for historical knowledge and understanding of the context of changes (searchable too!)
Github provides an awesome interface for this. Do you know how TTS hooks up to Cassius? Neither do I. But guess what? Kasey made a PR for his changes to the system (months ago) and you can check them out!…Oh! And there’s even some description about why the changes were made.
Ensuring the changes you’re making to the code base are of manageable size (and testable)
The larger a changeset gets, the more risk there is associated with it. If we have “story sized” PRs. We can more reasonably mange the risk of certain deploys, and therefore increase the quality of the overall system.
Improved understanding of features not related to the ones you’re currently working on
Pretty self explanatory. Maybe there’s a feature you haven’t rotated to, however, you get assigned a critical bug in that area. PRs combined with code review might give you a jumpstart to getting the issue fixed quickly.
Improved Call Response
If the site goes down and you’re on call, your knowledge of recent code changes (particularly those of high risk), quickly becomes invaluable. Code review and PRs help to accomplish this.
Other Things to Think About:
Pre-commit Code Reviews:
We’ve tried this on a couple occasions and it seemed to work well. When you’re tackling a large feature set or something that effects architecture, it’s a great idea to expose it to the team early in the form of a Pull Request (tagging it as “in progress”). Doing this allow the team to asynchronously submit feedback that may guide you to a better solution.
Using the Dev meeting to walkthrough larger change sets:
We did this with Tutoring (post-merge). I think we should extend it to any large changeset or extraction of feature. The important thing to note here is that these walkthroughs should occur earlier in the process so that the reviewers have a chance to really understand the direction and provide feedback.
TL;DR;
Code review is awesome in a ton of ways but it needs to be a part of the team culture to work. In this email, I am proposing we make it part of our team culture.