I believe not many teams or organisations leverage the full potential of code reviews. It’s a simple tool/process which brings so much discipline and collaboration within and between your engineering teams.
In one of my earlier organisations, we used the waterfall model and the code reviews used to happen at the end of the implementation phase. All the developers got into a room and did a code walkthrough, specifically aimed towards the engineering architect. We used to have hours of discussion on trivial topics and there were also times when we identified fundamental flaws in design and we had to build it all over again. This was after an engineering team spent weeks building a feature.
Like many other things, a good code review is part of the team culture. You should put in the effort to build it and fight to maintain it. The culture in your team is highly influenced by the culture your team brought in from their previous experiences.
These are some guidelines we try to follow in your team. Maintaining a good code review culture is everyone’s responsibility. PS: I use code review and pull request (PR) interchangeably here
Code review is not a bureaucratic stage before your code reaches production. It’s not merely a process by which you get a mandatory seal of approval from your peers. Coming back to the earlier example, a code review stage must not be aimed at approval from the engineering lead or an architect. If you lead a team, you make yourself the bottleneck and rob the chance of growth and learning from the engineers in the team.
Code revies is not a platform to show off your competence. It’s not a platform to burn down less optimal code or to have long debates on naming conventions. When a new engineer joins your team, she takes some time to get familiar with the codebase. She might make common mistakes and will have to learn lessons that engineers with more tenure in the team have already learnt. Use code reviews as a platform to help a new engineer grow and learn. Think of every code review as an opportunity to learn. Invite the newbies in the team to participate in pull requests. A lot can be learned by observing the pull request discussions.
Code review is the best platform for the engineers in your team to collaborate, share ideas and learn together. If you are the engineering lead, it’s your responsibility to set up a good code review culture within your team. If you usually take part in code reviews, you can practice these yourselves and set an example. You could also set these as the guidelines for the team.
Encourage comments in the form of questions or recommendations. Rather than instructing your colleague to do something, it’s easy and better to make a recommendation or to ask why a specific design was chosen. Use the question to drive discussions and come to an agreement together. Usually, what may seem right to you may be something that your colleague has already tried or thought through.
Separate weak recommendations from strong. Do not try to force your personal preference over your colleague. If you would have structured the log statement a different way, do not get annoyed when your colleague does not think like you. In the same example, if you have a standard defined for team or project, and if you see it being violated, you can move from weak recommendation to strong.
If it gets too hard to continue discussions over a linear thread, take it offline. Walk over to your colleague’s desk and talk about it. But, make sure you update the thread with the outcome of the discussion and decisions made if any.
If you see bullying or insulting in a pull request discussion, step in immediately. If you are leading a team, you do not want your team to lose the moral safety within a pull request. Do not tolerate rude or derogatory comments in a pull request. A good culture within your team is something to fight for.
Take the extra mile, but do not force someone to run a marathon. If you could do a little refactoring or cleaning up, even if it’s not part of the task being reviewed, encourage this. If it gets too big, create a follow-up task immediately and add it to the backlog.
Try to get the PRs closed as fast as possible. When a pull request is being reviewed, an engineer is blocked. Even if your teammate picks up a new task from the backlog, she would have to come back to the PR which breaks focus. If you are an engineering lead, it’s your job to make sure the blockers are cleared as fast as possible. Make sure your team understands reviewing PRs are equally important as working on tasks.
Share PRs across teams. Encourage your team to look at the PRs from another team once in a while. Also, invite others to look into your PRs. You could learn a new way to solve a problem you have been struggling with.
When you approve a PR, accept that you are taking on collective ownership of the code. Do not easily give away your approval unless you understand the PR and has clarity about how it works. If you are stuck, do some pair programming, or even want more fun, embrace the mob. Like I mentioned earlier, remove the blocker ASAP.