If you managed to build, or be part of a team that is infallible in every possible way; a team that doesn’t produce code with bugs; a team that has a hive mind knowing exactly what code is written, how it works and where it is located — then, you will be happy to learn, life will be easy. Everybody can commit to a single branch and you don’t need code reviews.
If you’re unfortunate (or lucky?) enough to not have such a team, you will likely have to come up with a strategy to prevent people from blocking each other, sharing knowledge and ensuring code quality. One way of doing that are code reviews.
As I see it, code reviews have two key purposes:
- Sanity checking.
- Knowledge sharing.
A second pair of eyes on a piece of code is good. These eyes can find bugs, point out potential problems and corner cases. After years of research in the wild, I have come to the conclusion that people make mistakes. Sometimes silly ones, sometimes serious. It’s human. Everybody makes mistakes.
Unless you are pair programming everything, it is likely that specific knowledge about your codebase will naturally concentrate in specific team members. This is a dangerous thing, because people get ill, quit their jobs, get hit by a bus, or are unreachable when the shit hits the fan.
In that sense, teams are like distributed systems: they ought to be designed to withstand failure. Would you build a distributed system without redundancy and failover mechanisms? Only if the system is not all that important. If it is, you need to build in redundancy. It’s the same with teams, and code reviews are one way of encouraging this knowledge sharing — creating redundancy in knowledge.
In summary, code reviews: good, no code reviews: bad.
Where do code reviews fit in?
When I was at Cloud9, we made it part of how we used our version control system and Github (but Mercurial and Bitbucket would work just as well).
Never commit to master.
The way of working that has worked well for us is the “never commit to master” approach (replace “master” with “default” if you use mercurial). As the name suggests, this approach dictates that you never commit to the main branch directly, the only commits on the main branch are merge commits.
So, the rules are pretty simple; there’s only one:
Never commit directly to the main branch.
Well, unless you have to fix this one litt… No! Never. Commit. To. The. Main. Branch. Not for big whopping new features, not for a small bugfix.
Why? The main branch has to be stable and ready to be released at all times. That means that all features and fixes merged into this branch have to be done. Even if in your model of development you do not have continuous releases, consider your team members. If they check out the main branch with your half-baked work that is inadvertently breaking things, you are now blocking them. I bet that would never happen with any of your work, but could you say the same about your team members?
Since we all use distributed version control systems these days, each developer has a local clone of the main repository. When you want to start working on a new feature or create a bug fix, you simply branch off the main branch. You work on this branch locally and push the branch to the central repository from time to time. Pushing the branch to the central repository both enables other team members to watch the feature progress and to try the feature out, but also is an additional back-up mechanism of the work in progress.
Every morning (or more often), you merge the latest changes from the main branch into your feature branch, to keep up with recent changes and to avoid large merging conflicts at a later time.
When the feature has been completed from a code and test perspective, it is time to issue a pull request with the main branch. GitHub and Bitbucket both provide very nice UIs for this purpose these days. In the pull request description, clearly describe what the feature/bug fix is supposed to do and reference any related issue. You can use the @username notation in the description field to assign the review to one or more people, who will receive an e-mail about the new pull request.
The pull request is a very convenient and light-weight tool to do code reviews. It has a “Diff” view which shows all the changes of the pull request compared to the main branch — so there is no need to look through each commit individually (but you can if you like to). Each line of the code can be commented on to give suggestions for improvement or ask clarifying questions.
Here’s an example from an older pull request to the open source Cloud9:
When everybody is happy about the feature — Bitbucket even has a per-developer “Approve” button — the “Merge” button is pushed and the new feature lands in the main branch. Ready to be released to the public. Recently both Github and Bitbucket added the ability to easily delete the now-merged branch from the repository to keep things clean.
A valid concern with enforcing code reviews for everything, even the tiniest of changes, is the impact of the productivity of developers. In the short term, code reviews slow down the process. Beside writing code, developers now also spend a significant amount of their time reviewing code of others. These code reviews have to be somehow planned, and there needs to be a process to ensure they don’t keep piling up.
Nevertheless, in the long term I do believe they are absolutely worth it. I’ve seen many bugs being found as part of code reviews.
Additionally, it’s a great way of keeping up with what’s happening within your and other teams. In my role at Cloud9, pull requests were an invaluable way of keeping up with development of the product. I didn’t have time reading through every commit made by everybody in the company all the time, but whenever somebody finished something, I was notified and could read through the pull request, diff and comments. You will find that introducing code reviews and pull requests that everybody can look at, there will grow more awareness of what is going on, what code is actively being worked on and how it works.
Seems like a good value proposition to me.
This is an adaptation of a post I did at the Cloud9 blog back in 2011.