I posted previously about why to do code reviews, and this post is a follow up to walk through some mechanics for doing them.
When to do a code review
A huge number of developers are using git, and a similarly huge number are familiar with Github. So with that as a basis, my opinion is the best place to do a code review is at the point of sending (or receiving) a pull request.
If you’re not familiar with the term “pull request”, it’s the mechanism that Github has enabled in their website for someone to request to merge specific changes into a branch of code. Bitbucket, Atlassian’s Stash, and other tools have similar concepts. In OpenStack we used Gerrit for this purpose (in fact, all their reviews are completely public and viewable at review.openstack.org), and the earliest tooling I remember hearing about and setting up was Reviewboard (even contributed a few pieces to it back in the day). Suffice to say there’s lots of options out there to help set up and put some structure around the process of doing a code review.
How to set up your code to be reviewed
Tooling alone will not make a good code review, it just enables it. What you are reviewing, or what you share to review can have a huge impact on how a code review works.
Some of the rules of thumb I follow for making a code reviews easier are:
- Keep commits small and focused
Each commit should have a message that describes the change, and it should be relatively orthogonal to other changes. It’s totally acceptable to have a pull request with just a single, small commit. Honestly, those are the easiest to review.
- Don’t make a lot of different kinds of changes in the same pull request
If you’re doing a sweeping refactor that changes the code through-out a lot of places, then consider putting up the follow-on changes that add functionality into a separate pull request. If you’re slightly refactoring and then adding some functionality, that can work, just beware of how much you’re adding in that can confuse the intent to a reader. Basically, try and keep to a “single topic” within all the changes in a single pull request.
- Keep the overall code for review well bounded
Sometimes there is no way to avoid massive changes to prepare for, or implement major architectural shifts. Nonetheless, Try and keep the changes as simple as possible and easily understandable. You want to avoid changes so large that it takes more than 30 minutes to read and understand the review, and I try and aim for most reviews to be able to be read and understood within 5 to 15 minutes.
- If there is other changes that are required, explain what those are and why
In lots of larger projects, you may need to make changes in multiple different repositories at once, or have multiple pull requests that depend on each other. Working on changes in OpenStack was completely like that. In those cases, make sure you do something to “link” together the set of pull requests so that the reader/reviewing can reference what you’re expecting. A reference to the other relevant pull request(s) are often all that’s needed.
How to review someone else’s code
First and foremost, remember that a code review is about people communicating. Your culture WILL show through. In my teams, I lead and encourage folks to communicate objectively and positively. How you phrase and communicate will be as important as what you say.
First, read through the code (and changes) that are being suggested. Make sure you understand the intent of the change, and how it was solved/implemented. If there’s some arcane bit of perl-ish anonymous function code that you don’t understand, ask about it. Most of all, understand what was intended with the changes, and how it was done.
Second, look for architectural patterns:
- Do the changes match up with how you’ve generally been structuring similar changes?
- Does something fly in the face of the architecture that you’ve set for the surrounding code?
- Does the implementation break a hole in an otherwise clean interface and make an API into a leaky abstraction?
Third, look for the stylistic details:
- Do the variable names make sense?
- Does it follow the coding conventions you’re using?
- Are there any obvious misspellings?
- Do the comments and internal code documentation annotations match the functions?
Last, review the continuous integration results:
- Is it passing the tests? For that matter, does the new code HAVE tests (assuming that may be important to you)?
- Did the overall code coverage of the project go down?
Very often, these pieces are answered by automated continuous integration tooling. If you don’t have continuous integration, check into it: maybe TravisCI and Coveralls or Jenkins with the violations plugin combined with the Github Pull Request Builder.
Communications Style
Most of the horror stories about bad code reviews that I’ve heard (or been subjected to) have been about bad communications, not bad code. Objective and honest is excellent; questioning and discussing is good; personal attacks, verboten. I’ve searched for the best way to describe what I mean here, but I think Ed Catmull did the best job in Creativity, Inc.: be candid.
While I’m on the subject, one of the best things I’ve found for writing in reviews was taking a leaf from one of Pixar’s books. Ask yourself, “How do I plus-one this?”. Even if the code looks fantastic to you, it’s worth taking the time to read it through thinking “Yeah it works, but how could we make it better?”. You don’t need to go overboard here – in many code reviews, I honestly just say “Yep, looks good to me”, but it’s still worth the thoughts.
Rhonabwy 