Ármin Scipiades | September 8, 2020
6 Minutes Read
Code review has become a pretty widespread agile technique in software companies, but it’s often misused. Sometimes it becomes a competitive sport between developers. Sometimes it’s just the lead developer correcting their subordinates’ work as a teacher corrects homework. Sometimes it’s just a glance over the pull request (PR) before merging, a mandatory and useless chore. Sometimes it’s even dropped altogether as a waste of time. In this post, I’ll talk a bit about our reviewing habits in our team here at Aliz, since it became a part of our company culture.
We do mean code reviews. We’re sort of hardcore. Every developer on the project is expected to participate, more or less from day one. You may feel you don’t have anything meaningful to say (I certainly felt that way!), but you actually do. If you don’t understand a comment or a piece of code, that alone is an issue. We’re building a large system that’s going to be worked on for years to come, with code needing to be revisited years later. It’s imperative to ensure that all our developers will be able to understand it. And participating in the code review is a great way to get to know the codebase. It immerses you in the team’s coding style, practices, and values. You’ll also see the other reviewers’ comments, and this way you’ll learn the ropes. There are no hard-and-fast rules about how much time you should spend reviewing, or how often, but you’re expected to make it part of your routine.
Reviewing for fun and profit
We use Reviewable, a pretty cool tool fully integrated with GitHub. I check Reviewable whenever I have some downtime. If I see a new pull request listed, I check it out: this helps me stay up to date with changes. It’s like keeping a finger on the pulse of the project. And if I spot some obvious issues, I point them out right away: quick feedback is important.
I also try to set aside an hour or so every day to review PRs in a dedicated way:
It’s best to review a PR you weren’t much involved in. If you’ve helped to design the solution, you won’t spot the defects in it! But of course, if you’re reviewing a PR about a part of the code you know absolutely nothing about, well, then that’ll take a lot of work on your part. And you’re most likely won’t be able to spot deeper issues, but that’s okay. As with any agile process, the trick is that we can ditch it anytime. If I really feel out of my depth, I’ll ask someone more experienced to review it as well.
The joys of being reviewed
So, your work will be reviewed. Problems, faults, and uncertainties will be found, but that’s to be expected (the natural state of code is that it’s buggy). Sometimes it’ll mean rewriting and redesigning half the pull request, and that will be jarring. There’s a chance you’ll feel criticized, maybe even hurt.
But it’s important to realize that it’s not you that’s being reviewed; it’s all about the solution you are creating. Your competence is not being questioned. You’re not being critiqued. Your reviewers aren’t out to find fault with you, but with your code; they want to help you. It’s all about making something together. It may take some time for you to adjust to this mindset, especially if you come from a very hierarchical company culture.
Being reviewed may be rather difficult at first, but the rewards, oh boy, the rewards are just awesome:
Pitfalls and precautions
Okay, enough of the gushing. The code review as a process is not without difficulties and pitfalls.
For one thing, code review breaks if it’s not a discussion of equals, if hierarchy comes into play. You shouldn’t just believe anyone to be right. You shouldn’t change your code just because they tell you to; you should always seek to understand the whys. Be assertive when reviewing and when being reviewed. This is sometimes difficult. It takes guts to tell a seasoned developer they’re misusing a function, or to tell them that no, that suggestion is awesome, but really shouldn’t be crammed in this issue.
Because, yeah, code reviews sometimes lead to improvement creep. I’ve been guilty of this. When it’s not an urgent fix, it’s easy to lose focus in wanting to improve the PR more and more. Again, be assertive and focused.
There’s also a risk of disagreements going nowhere. These usually come about as a result of the reviewer not communicating clearly enough, or the reviewee being a bit slow that day. Luckily, this rarely happens. If there’s any misunderstanding or disagreement after the first exchange, we quickly call each other up and discuss the issue in person. If we still can’t resolve it, we call in a third party to help us decide. It’s important to document the final decision in a final comment in the review as well. Reading review comments of past PRs is a good way to find out more about an ancient issue.
Another common complaint against code review is that it takes too long to get someone to review the thing. We have this problem sometimes: if your PR is big and intimidating, then people will be wary of even starting to review it. One way to combat this is to split it up into smaller commits (which is something we all should strive for anyway), or into multiple PRs. It also helps if you have the guts to mark someone as a reviewer: people rarely ignore being called out like this.
Still, late reviews are a problem, especially since it forces frequent context switching for the developer.
Asynchronous, scalable, automatically documented pair programming
Code review is not a silver bullet, a magical solution to fix all problems. It’s but one technique used in software development. It requires a strong company culture, and it’s easily misused.
The obvious alternative is pair programming, which (when done right) is something like code review on steroids. But unlike pair programming, reviews are asynchronous and don’t need a physical presence. Pair programming also doesn’t really scale: mob programming is fun and all, but hardly efficient. The asynchronicity of code reviews makes mob programming an actual possibility! All this meshes well with teams working remotely, on flexible working hours, and that’s a big deal during the current pandemic.
Code review is more conservative than pair programming and trunk-based development, better suited to work on large, legacy systems. For example, the results of the review are automatically documented, while discussions during pair work are lost.
Code review works for us, sort of, most of the time. And I quite like it.
How about you?
Buon giorno, Milan! Aliz opens 4th office, this time in Italy
3 Minutes Read
Ciao, Milano! Celebrate the opening of our new office in Italy with us and find out why we’re so excited about setting foot in Milan. Read our blog post announcing the news:
Google Cloud Infrastructure Modernization - Stay Agile With An Open Architecture
3 Minutes Read
Learn more about how Google Cloud infrastructure modernization solutions can help your business to become more competitive.
Everyone can do ML on GCP
3 Minutes Read
In this post, we will look at how GCP brings ML to the masses through premade APIs, AutoML, BigQueryML, and AI building blocks.