Code review culture at Aliz: why and how we review code

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.

Hardcore reviewing

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:

• I first read the Jira issue prompting the PR, and try to understand the business case. If the issue description isn’t clear, that’s a problem in itself.
• I check the interface: method names, signatures, Javadoc. If it’s unclear, I leave a comment.
• I scan the implementation, pointing out elementary errors right away. I also leave a comment if I see anything I don’t understand. If I don’t understand why a piece of code is the way it is, chances are the next developer working on it five years later will be just as puzzled. Let’s be courteous and leave them at least an explanatory comment.
• I don’t only point out flaws: When I see something impressive, I leave a complimentary comment.
• Then things get tough. Are the edge cases covered? Will it be easy to maintain? Is it performant? Is there a better way to do it altogether? Are the tests good? I mean, not just okay, but good? This is the hardest part. It takes quite a lot of mental effort, as I have to rethink and maybe redesign the whole problem.

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:

• Bugs will be found. Sure, you’ve double-checked your code, reasoned about it,, and written all the tests, but you’re just one person. Even when looking at the same piece of code twice, you’re seeing it with the same preconceptions you had when you first wrote it.
• Your code will most likely get better. It’ll be easier to read and reason about, mesh more with the team’s coding conventions, and generally be prettier to look at.
• The responsibility spreads out. When the issue comes back from QA, or heaven forbid from production, it’ll help to ease the feeling of guilt that yeah, others reviewed it and found it okay. It helps to build a company culture where the team shoulders collective responsibility, not pointing fingers when things go wrong.
• The bus factor decreases. If a developer is hit by a bus (or just goes on vacation), the team won’t be hit that hard. Sure, maybe they knew that part about fooing the bar really intimately, but hey, you’ve reviewed the PR in which they did that tricky refactor, and you generally know how it’s supposed to work.
• Knowledge transfer gets easier. You see that cool pattern others use, and you start using it as well. Since you’re seeing it in action, it’ll leave more of an impression than if you were to see it in a presentation.
• Your code style will naturally meld into that of the team. We don’t even have a detailed style guide; it just happens naturally.

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?