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.
Hardcore reviewingWe 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 profitWe 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.
The joys of being reviewedSo, 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.