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.
11 replies on “Code Reviews”
I have some more things that I consider important in a Code Review session:
What is the context of the change? Why the change? Why the refactoring? (commit or PR)
* Does it stand on its own? <– I think this is important
What have you learned during doing the change?
Maybe add that as a commit message.
Focus: What should we be reviewing?
What tool can we leverage to make the review more automated?
Social aspect: How do we handle disagreements? As a team.
Review not only code in your team. What about the build process? Deployment scripts? Configuration setup?…
Thanks for the blog post
I would hope the context is clear when the work is started, but I agree that that’s a good think to revisit to set the scene for the review. Thanks for your contribution.
What do you mean by stands on its own? As in, the code under review is a complete new feature, or that it is self-consistent (code and tests match, etc.)
Does the change need a lot of explanation via voice? Or is everything there, so that a future reader can follow everything aka the Why? What?
LikeLiked by 1 person
Your blog has a funny comment feature. Somehow it messes my comments a little up…
Sorry. I’ve fixed the formatting for you.
Sorry about that peitor – it’s she standard WordPress comment functionality. 😦
[…] thanks to @peitor for engaging with the last code review post, particularly his comments there, which I recommend reading, and his […]
[…] my code review post, I added some more thoughts regarding code reviews, but there was one comment that I wanted to come […]
[…] from the team. Make green builds a mantra, and dress up as a dinosaur to enforce it if you have to. Make everyone care about code consistency. Make everyone care about the quality of each other’s […]
[…] a technical lead, I think code reviews are essential to ensuring the quality of the code. On a project I worked on, due to changes in Project Management, I ended up switching between code […]
[…] Peer review is an essential component of a functional team producing quality software. It allows knowledge transfer, stops obvious and sometimes not-so-obvious bugs reaching production, and it aligns everyone to process and coding standards. […]