Valio.fi deep dive #2: Code review policy
In this second episode of my postings on valio.fi, I’ll look deeper into the concept of code reviews in the project.
Pre-post note: No, I’m not following a logical order with these. Rather, I’ll try to answer the asked questions as soon as I can while still maintaining a readable approach to the project details. Front-end posts will be interleaved with back-end magic, and there will be a dose of project management sauce inbetween. If you want to hear about something, ask for it in the comments.
In a project with numerous developers – in our case a scrum team of nine – the question of spreading code knowledge and ensuring certain quality bar tends to arise. There are numerous ways to tackle this, but we picked two principal approaches: Shared physical working space and code reviews.
Peer reviews vs. architect reviews
There are basically two alternative approaches to picking the reviewer. Either you have a reviewer/architect role (which may be rotating, but often tends to stick), or you can have a system of free peer reviews, i.e. anyone can review anything as long as they feel sufficiently competent about it.
Wanting to emphasize the equality of team members, we naturally chose the peer review model. However, few teams have a totally balanced review graph. Usually some people end up reviewing more code than others. This is partially a function of seniority, but also dependent on character, preferences and random factors (such as knowledge of a particular tricky subject).
Ours wasn’t balanced either. A couple of persons ended up being a part of most of the reviews. Still, we found the system to be fairly optimal: it did distribute a lot of knowledge, everyone had their code reviewed by several others and everyone reviewed somebody’s code.
Review guidelines
As with any project with more than a few people and a duration of months or more, writing down the intended methodology felt wise. Thus, we did have written review guidelines, although they were just that – common sense was allowed to rule over these idealistic methodology statements. Here is a summary of our guidelines with some examples and clarifications in parentheses:
Changes that must be reviewed
- Changes to commonly used base classes
- Changes to tool libraries (defined as a group of Visual Studio projects)
- Changes which alter the architecture (add new layers of abstraction, create new dependencies) or institute new technical vocabulary (e.g. introducing the concept of “distributed cache eviction handlers”)
Changes that are recommended to be reviewed
- Changes which touch critical, bug-sensitive code paths (O/R mapping internals, request routing, complex logic)
- Changes which seem incorrect or illogical by the first reading of the code
- Changes whose unit test coverage is insufficient but hard to improve (I’ll get back to this in a later post)
Rules of play
- The author of the code makes the first call on whether to request review. Anyone else in the team may also request a piece of code to be reviewed if he feels it necessary.
- The author may request the review from one or more people.
- Anyone may review, but authors should usually pick the one most experienced in the domain of the change.
- Reviewers can ask for additional reviewers if they so feel.
- Reviews should be handled in a speedy manner. A typical review request should be completed in a working day.
- The author must answer any comments or questions raised during the review in a speedy and prudent manner, but it is ultimately the author’s choice to either honor or ignore them. Should significant disagreement rise, the team will solve it together.
Some notes on the guidelines
These rules, as written above, were applied at the start of the project. During the final few sprints, we relaxed the guidelines and focused on shipping.
No, wait, maybe that was what you expected to hear. Actually, we tightened the rules a bit for the final three sprints (six weeks) or so. Mainly, we merged the two first headings; essentially, we started to require reviews on all non-trivial changes to the code base. We also pushed this to our project management system; a sprint backlog item was not marked complete until it had passed review (for whatever definition of “passed”; we still allowed the author to decide whether review comments warranted code changes or not).
As you can see, we relied a lot on code authors. We allowed everybody to request review from anyone, meaning that we didn’t – nor did the project management – get involved in how much time was spent in reviews. And quite some time was spent, although we didn’t log any specific numbers. Personally, I spent almost half of my time reviewing code towards the end of the project, although admittedly I was one of the most active reviewers.
Forced vs. recommended: Before or after checkin?
There are basically two stages when you can do a review. Many open source projects take a fairly strict path: Only a select few people can commit or authorize commits to the source control, thus essentially creating a tight net for reviews. Often, in the largest projects, nobody commits anything without somebody else looking at it first. This makes sense when nobody can know the code authors. The other alternative is to allow free commits, but review before releases or other milestones.
We were totally in the post-checkin camp. Everyone could commit anything, and we had no system in place to track if people actually did request reviews (until late in the game when we made it part of the completion criteria).
If you have the right culture, reviews are free
So yeah, we had a very author-controlled approach to reviews. But we also had a very open code quality culture, and it was not uncommon to question the validity of somebody’s approach over lunch. The debates could get quite heated, and we often spent a lot of time on issues that didn’t, in the end, have enough weight to warrant such use of time.
However, it all served a purpose. Our somewhat excessive quality discussions created a culture where writing bad code – or skipping reviews – wasn’t particularly lucrative. You never got publicly lambasted for a stupid bug in unreviewed code, but everybody felt better collectively owning code that had passed a review: a bug in well-reviewed code was a team mistake.
The reviews definitely had a time cost. Their positive impact is very hard to measure, given that we couldn’t know what the amount of bugs would have been without the review net. We caught some in the reviews, but probably prevented ten times more by conveying knowledge and setting examples during the review process.
A couple of weeks after shipping the site, many people kept wondering about the low amount of bugs discovered right after the launch. This sentiment, no matter how unmeasurable, is one of the key indicators of our success in terms of quality.
A perfect success?
Far from it. First of all, our front-end code wasn’t nearly as meticulously reviewed, for a variety of reasons (although the same attention to quality did apply, it wasn’t as collective). There were definitely scenarios where reviews could have saved us from some front-end issues.
Then, there was the question of review culture. While we were mostly quite successful with it, we could’ve been a bit more stringent with the adoption at the early stages. Given that the team was formed from three organizations (with three different review backgrounds), we should have made everything clear from the get go. It would have been a step away from self-organization, but we would have set a meaningful default, then allowing the team to find its own path. At the very least, review completion should have been a part of the “done” criteria from a much earlier point of time in the project.
All in all, reviews made a big difference in the project. One more thing we could’ve done better is follow-up: We should have gone through common review comments more often and intentionally disseminate information about the issues on a weekly basis. We didn’t have a process for that. In a bigger team and a more intense schedule, this would have been even more important.
Up next: Tools
That’s it for the review methodology. The next post will be about the “how” of reviews, including the tools (Crucible). At a later stage, I will return to the topic of reviews by explaining a few of the common issues we found and giving some ideas on how to tackle those issues.
April 28, 2011
· 
Jouni Heikniemi · 
3 Comments
Tags: code review, valio.fi · Posted in: Misc. programming
3 Responses
Tero Teelahti - April 28, 2011
Not loosening peer review criteria towards the end (shipping) is a great accomplishment that I envy right now…
Heikniemi Hardcoded » Valio.fi deep dive #3: Review tooling - May 4, 2011
[…] my previous post on review policy, let’s have a look at the tools we used for reviewing […]
Klinik Kuret - December 6, 2019
I was wondering if you ever thought of changing the layout of your blog?
Its very well written; I love what youve got to say. But maybe you could a
little more in the way of content so people could connect with it better.
Youve got an awful lot of text for only having one or 2 images.
Maybe you could space it out better?
Leave a Reply