At our last dev meeting, we discussed code reviews. We do code reviews for all code on all projects, before code is merged to the master branch, and everyone is familiar with doing them, but we can always improve how we do things. So, we had a general discussion on what good code reviews are, and how we can do them best.
We initially discussed what one should look for in a code review. We agreed on some questions a code reviewer should ask themselves:
- Do you understand the change being made, and more importantly why it’s being made?
- Is the change itself good? Is it clear, are things well-named? Is it testable; and does it have tests? Does it introduce any non-functional issues such as security or performance problems?
- One should look at what’s been done, but also the context of the change. Is there code elsewhere that should be reused? Are there similar changes to make elsewhere? Is there now obsolete code that can be removed?
- Even if the change itself is fine, has the codebase as a whole become less clearer? Has a file or method become too big?
We talked about some of the benefits we have had from good code reviews, such as:
- Finding out better ways of doing things – such as being able to change a large chunk of procedural code into much more concise and clear functional code, and new ways of doing legacy things
- Improving consistency throughout the system, using common approaches such as use of optionals, and putting code in the correct classes, clustering similar methods close to each other
- Getting the benefit of other peoples’ wider experience – e.g. new people have new perspectives and approaches that they have from previous companies, that we can learn from
- Sometimes it makes sense for the reviewer to make changes and improvements to the code, rather than just handing it straight back to the original developer
- Identifying areas that are unclear or confusing in the existing system as a whole, and that need refactoring
- Helping standardize our practices within and across teams, so we can improve generally
As a reviewer, you are aiming to do a good code review, but as a coder, you also have a responsibility to help make the code review useful as well. These include:
- Asking for a design review before starting the task at all
- Making useful, atomic commits, with meaningful commit messages, rather than a big bulk commit of everything followed by a few “fixing this” commits
- Making tests clear and readable, and avoiding repetition in them, just as we aim to make the main code clear and readable
- Review your code yourself first, before you commit – stepping back from the moment-by-moment coding often helps you spot issues. This incudes the basics – check that the code compiles and relevant tests pass
- Asking for partial review of long tasks, rather than waiting until the whole task is complete. Frequent review and merge makes the process simpler
- For a long branch, being ready to fix issues on that branch quickly if a reviewer find any, e.g. by reviewing other peoples code or by doing short tasks, rather than jumping into another long task
Ultimately, it is the reviewer that is doing the review, and we discussed the ways that a reviewer can improve the quality of their reviews:
- Consider whether you are the right person to review the code – usually it’s good to spread reviews around the team, but sometimes you might want an expert in a particular area to review the code
- Being able to start the review quickly – so prioritizing code reviewing over starting on new tasks
- Providing all the feedback from a review at once, rather than in dribs and drabs
- Pointing out what’s good, as well as what’s wrong in the code – this is good for morale, and also encourages good practices
- Providing a rationale for feedback – never just “do this”, but “this is a better way because…”
- There can be multiple issues with a piece of code – sometimes you’ll be initially distracted by minor points, but might need to revisit the review once those are resolved to spot fundamental problems of approach
- Pointing out alternative approaches that might be useful in future, even though a change isn’t needed in the current review
- Avoid giving feedback that is impossible to apply – as a reviewer, think about how you might solve a problem yourself, and discuss the issues with the reviewee
And after the review, for all the items raised, each should be discussed and either resolved, or agreed to be not needed now, or a ticket raised for them to be fixed soon.