Submitting a Good Code Review
As I previously mentioned, I am a big fan of code reviews. I love doing code reviews for my coworkers, and I love having them review my code. Having been doing them off and on since 2000, I’ve picked up a few tips for submitting code to be reviewed that really help. How you submit your code review can make a big difference in how easy it is for the reviewer to understand what’s going on, and in how many suggestions they have for changes that need to be made.
Before going into this, let me provide a little context on how code reviews work at Snagajob. We basically use the standard github flow. When developing code, I work on a branch. Then I submit a PR to have that branch merged into one of our mainline branches. The PR is published into slack and anyone can pick it up and review it when they have time. Now and then I’ll ask a specific person to review a PR for me if there is some particular domain knowledge necessary, but usually I’ll just let any API developer review my API code, or any web developer review my web code.
One of the things that I believe is most important, that often doesn’t happen, is to actually look at the code you are submitting. This may sound silly. You just wrote that code. You know what it does. But I’ve made it a habit to skim through the diffs right before submitting the review, and it’s surprising how often I notice things that aren’t right. Maybe a bit of debugging code got left in accidentally. Maybe I submitted a giant block of code and forgot to put in any comments. Maybe the file I deleted refers to another file that can also be deleted now. You don’t have to spend hours reprocessing every detail. But it really helps to just do a quick read through of all the changes.
While looking at your code, think about what you would say if you were reviewing it. Would you point out some missing unit tests? Would you ask for more comments? Would you be able to understand the overall logic? Would you complain about where it’s not adhering to coding standards? Would you spot all the places that are missing a null check? Might as well clean those things up before asking someone else to take a look. It’ll save you both some time, and you’ll give a better impression.
Try to make your code clear and understandable. Just because you understand what you meant doesn’t mean someone else will. Make sure to include comments, name your methods, variables, classes and other tokens meaningfully. This is helpful for the reviewer, and also helpful for whoever has to maintain this code in the future (including yourself).
Separate dependency updates from actual code changes. Whether you use nuget or composer or some other package manager to pull in dependencies, the person reviewing your code doesn’t need to read through a bunch of config file updates. Reading through a C# Pull Request when there are 10 lines of changed code and 100 lines of version number changes in various packages.config and *.csproj files is annoying. At Snagajob, it is recommended that nuget updates be made on the mainline branch (after all, the code in those version updates has already been reviewed elsewhere), then merged into your branch, so the only changes the reviewer needs to look at are your actual code changes. Now and then, a package update includes breaking changes that require code updates, and so this will need to be done on a branch. But that is the exception rather than the rule.
Avoid monolithic reviews. If you are working on a feature that takes a week of work to complete, it should be broken up into a series of smaller reviews that are meaningful independently. Maybe submit a review for the database code, one for the backend business logic, and one for the frontend. Or maybe submit separate reviews for different features within the project. However you do it, make sure that each submitted peice of code makes sense on it’s own, works, and doesn’t break existing functionality. There are two reasons for this. One is that when you put out a giant review, you are going to have to beg to get someone to look at it, and once you do it will probably take them a while. The other reason is that it may save you a ton of rework. You may get comments on that first review that point out a fundamental flaw in your architecture or assumptions, or that just help you come up with a better way to do things. Better to find that out after a half a day of work rather than after you have a week’s worth of code that needs to be refactored.
Now that you’ve got the right code in your review, you also need to provide all the necessary information — goals, context, assumptions — for the reviewer to understand what you are doing. Give them an overview of what you are trying to achieve (maybe even a link to your story card in JIRA or Trello or whatever tracking tool you use.) Explain any assumptions you made in this code. Perhaps there is validation code already in place, so you know that data is coming into your new code in a very particular format. Your reviewer may not know that the strings are all lowercase and waste time (theirs and yours) leaving lots of comments about how you need to change your code to be case-insensitive. Is there a larger context they should know about? If you broke up a big project into lots of smaller reviews, it might be helpful to give a quick summary of the bigger project. Try to anticipate any questions that someone unfamiliar with what you are doing might have, and then answer those questions. While doing this, it’s also worth thinking about whether anything you are saying would be helpful to people looking at and maintaining this code in the future. Maybe it should be comments within the code base, rather than just on the review request.
Got any important tips I failed to mention here? Please leave comments on your best practices for submitting code reviews.
Originally published at engineering.snagajob.com on June 30, 2016.
