The Github pull request view.
| « When To Write Bad Code | Using Interfaces For Exceptions » |
The Github pull request view.
Improving the overall code quality is a challenge. You work really hard at writing quality code, but bugs still slip in and code isn’t quite up to standards each and every time. It’s frustrating to you that there’s not a way to catch some or all of these issues beforehand; you’d love the input of others and the satisfaction that having your code reviewed brings. But how do you get a code review process started?
On my team at Mozilla, we review each and every patch before it’s committed to master. The process is simple: the developer commits their code to their branch on Github, issues a pull request to master, asks the person who they would like to review it, and waits for their patch to be reviewed. This system is simple but effective in ensuring that code is of high quality and reduces the number of bugs that go into master by putting another set of eyes on the code.
Here’s how you can get a code review process going for your team.
Make the process as simple as possible.
Complex processes are doomed to be discarded, not adopted. If your process is complex and difficult to implement for your developers, they’ll resist using it, and their resistance will almost surely doom it. Make the process easy to follow, and as few steps as possible.
Use Github pull requests and private repositories.
Github has the best user interface for code reviews I’ve seen. By being able to annotate individual lines and leave code comments, you have the ability to truly review code line by line. The pull request process ensures that developers can have their code easily integrated after review is complete. Private repositories are a cheap way for your company or team to have the user interface of Github without making your code public.
Spread out the review work so nobody is overwhelmed.
The hardest part of a code review process is having only one or two people that can review the code. Even if only one or two people have commit access, it’s important that most if not all of the team be able and assigned to review other developer’s code. Not only does it improve a developer’s skills to read through another developer’s code, it encourages team development by having developers work together. Plus, by integrating the whole team, no one or two people feel overwhelmed.
Have a weekly triage of requests needing review.
Review needs sometimes fall through the cracks. This has happened on our team from time to time and we have solved this by adding a weekly triage and review of pull requests during our weekly status meeting. This doesn’t need to be done more than once a week as developers are likely to request review and follow up on important bugs; it’s usually the big, difficult patches that don’t end up getting reviewed in a timely fashion. This weekly check-in will help.
Have a simple system for approved and rejected.
On our team, a pull request is either “r+” meaning “approved”, or “r-” which means “not approved.” An “r+” can come with considerations: “r+ with the following nits to be fixed before merging…” for example. An “r-” should ALWAYS explain why the item was rejected and how it can be fixed (if it can be fixed). Having any more of a complex system would frustrate developers.
Brandon Savage is the author of Mastering Object Oriented PHP and Practical Design Patterns in PHP
Posted on 1/24/2013 at 7:00 am
cordoval (@cordoval) wrote at 1/24/2013 2:35 pm:
what is nits?
Brandon Savage (@brandonsavage) wrote at 1/24/2013 2:36 pm:
Nits are small problems in the code. Typically nits can be resolved without a second review. For example, if you’re writing Javascript and are inconsistent about semicolon use, the reviewer might tell you to make that consistent and approve it “with nits” meaning “with small changes that don’t need a second review.”
Thomas Ploch (@https://twitter.com/tPl0ch) wrote at 1/25/2013 1:54 am:
I hav introduced the GitHub PR workflow with bi-weekly review of critical patches. The best thing that you forgot is that you can trigger automatic builds for PR commits on your company’s CI Server.
Mute wrote at 2/3/2013 5:02 am:
We have been using phabricator (http://phabricator.org) and its really really good. The arcanist workflow and cli tool makes working and reviewing much easier, for us it worked better than github.
If anyone is considering making code reviews the cornerstone of their development process I would highly suggest they give it a try.
A basic example of the workflow would be:
git checkout -tb private/mychange
…do changes
git commit -am”blah”
arc diff (creates a review request)
… wait for review, when its accepted
arc land –squash (apply the change to master and squash the commits)
| « When To Write Bad Code | Using Interfaces For Exceptions » |