Published by marco on
A healthy and active review culture is essential for any team interested in building quality software. At Encodo, we’ve been doing reviews for a long time. They’ve become an essential part of everything we do:
What we mean by review is not a formal process at all. It is simply that you prepare work you’ve done for an informal presentation to a team member. Explaining what you’ve done in a review is often a good way of collecting your thoughts—you should be able to explain what you’ve done. Getting a review from a colleague is an efficient and productive way of making sure you can do that.
While there are many reasons to do reviews, we’ve also learned that reviews can’t do everything.
It’s important to get reviews often enough to avoid wasting time and effort but not so often that your work or the reviewer’s work grinds to a halt. It’s all about balance.
A good rule of thumb is about one review per task. If your task is longer than a day, then think about how to break up that work into phases in order to get a review of earlier phases.
That way, you’re more likely to catch issues before building on top of mistakes.
Encodo prefers live, face-to-face reviews.
This is the most efficient manner of reviewing as neither party has to prepare anything other than the work to be reviewed. Issues that come up can often be handled immediately—and such issues are far more likely to be mentioned and fixed. While in-person reviews are superior, video-chat/shared-desktop reviews work quite well, too.
If that’s not possible, then we have also used tool-based, asynchronous reviews, such as pull requests with review software. However, we find these to be not only less efficient but also less likely to find as many issues.
With a live code review, it’s relatively easy to ask the submitter to reorder, split or squash commits. It’s also easier to point out and quickly fix stylistic issues (like naming or interface usage, etc.). Because the turnaround time is much faster, a reviewer is far more likely to point out smaller fixes that would improve code quality, maintainability and so on.
However, in an asynchronous review, a reviewer must decide what is most important. Is it worth rejecting the whole pull request if it’s 95% correct with a few details? Do you reject it and ask the submitter to fix up spacing or formatting or missing documentation? Do you really write down every last little thing you would have said? Do you reject it and hope that the submitter understands all of your notes? Or do you accept it and just fix those things up yourself? How many iterations do you go through?
We prefer synchronous, face-to-face reviews because they’re much more efficient. Misunderstandings can be cleared up quickly, iterating until the submitter and reviewer find a consensus.
We encourage reviews everywhere because we know how to make them faster.
Both the reviewer and the submitter need to practice. A reviewer should practice diplomacy and formulate critique in a way that it will be accepted. A submitter must keep an open mind and prepare good arguments or justification for the code. Both sides should stay positive. A review shouldn’t be a competition: it’s about producing high-quality code together, as a team.
Encodo has done presentations on reviews, in both English and German.