Code Reviews

In my thoughts about codecraft post, I mentioned that we’d discussed Code Reviews during the lean coffee session:

The question was “How much should we nitpick in Code Reviews”, and I can easily see Code Reviews being a great guided conversation on its own, especially with the side discussions on what code reviews are for, who should do them, and when they should be done, which reflect conversations I’ve been having at work.

It’s a topic that’s stuck with me, so I wanted to add a few more questions, to see what you think, and some thoughts about code reviews from my experience.

What are they for?

The common refrain around code reviews is that they ensure consistency and quality of the code, and should be seniors or peers checking the code against a checklist (possibly implied), following a process to ensure that the mistakes that came before aren’t made again. I can hear the agile proponents in the audience screaming at that.

Alternatively, the code review is a structured communication about the problem, and the solution, and the architecture, where the developer feeds back their understanding of the requirement and their implementation of it to another member of the team to validate their understanding, and through that conversation, with another pair of eyes, the developer and their pair can uncover any potential issues that need to be addressed. At the XP end of this scale, this code review is a continuous progress and a natural side effect of paired programming, and in such a view, any review that requires less time than the original development is a poor substitute for a full conversation.

Code reviews can also be used to gate check-ins – to ensure that all code has been viewed by at least 2 (or sometimes 3) pairs of eyes before it is allowed into a branch on the release path.

How much detail should they go into?

If a code review is a conversation, the answer is easy : as much detail as the developer went into, if not more. In the checklist view, the level of detail depends completely on the checklist. Hopefully, the checklist itself is enlightened enough to transfer some of the checking to automated tools, to enforce coding standards, test coverage, and other things, and the code review is the recognition that those automated tools are never enough, and an attempt to plug those gaps.

How long should they take?

As long as they need. It should never be a burden or a blocker to the rest of the team. If code reviews are a bottleneck, there’s something wrong, but equally, a code review that only takes a couple of minutes is a paper exercise, on anything but the simplest change.

Should the developer be present?

The situation is obviously complicated by geographically distributed teams, but let’s assume for this argument, there’s a decent video conference set up for the conversation. The developer has the opportunity to introduce the requirement and the implementation, and the reviewer can ask questions to clarify decisions. Ideally, these would be open questions, that elicit deeper responses from the developer, and may often be generic questions, such as “How does this work?” or “Why did you do choose to to it that way?” that could, if required, be put on flash cards to help new reviewers.

Tools like Github (and gitlab) are set up to allow asynchronous communication over a code review, which lessens the need to a developer to be present, but may depersonalize the experience to some extent, which may provide a more complete check, because the review is of the code rather than the developer, at the expense of potentially alienating developers who don’t agree with the changes. If the developers can’t work together, they can’t review.

If reviews frequently lead to rework, direct conversations are definitely more important to resolve ambiguities and ensure the developer being reviewed understands the reasons and consequences.

Should they be done at the end or the start of the feature?

This was a way of working that I discovered from Gitlab, where many pull requests are created at the same time as the feature branch, allowing conversations between developers as the code is developed, rather than using code reviews to gate check-ins prior to merging. This allows architectural and other design decisions, and the requirements, to be discussed in situ, with the code as it is being developed, which should inoculate the branch from the worst effects of drift that may occur once the Continuous Integration ideal of everyone integrating onto the same branch multiple times a day is compromised to allow feature branching.