There’s a lot of guides on the best practices for code reviews but in practice it’s very hard to stick to those ideals. In the real world, teams are under pressure to deliver which leads to a decline in quality without solid processes in place.
In this post, I will try to set out a simple guide on how to implement beneficial but practical code reviews into your team’s development process.
Outline
- What are code reviews and why do you need them?
- Before you submit your code for review
- Doing the code review
- Implementing code reviews into your existing processes
I will occasionally reference technologies such as git as well as concepts such as a story from agile development. This is written more from a point of view of code reviews in web development, but I’ve tried to make it applicable to all areas.
The what and why
If done right code reviews (CRs) are one of the best tools to ensure code quality and maintainability. The main benefits are:
- Better quality PRs: Having all work go through a CR forces developers to slow down and think a little more about the quality of the code they are pushing. There are many things devs accidentally push like TODOs, commented out code, missing unit tests and other things that could at best clutter the code; at worst break a production website.
- Sharing knowledge and upskilling: CRs provide a great platform for developers to learn from other more senior devs or ones with more domain knowledge:
- Junior devs can get guidance from more experienced team members which includes advice in a real-world setting.
- They allow devs to gain a level of understanding around pieces of work that they didn’t directly work on. They also allow devs to suggest existing functionality that the committer may have not known existed. - Consistency: The code being reviewed can be checked against the team’s decided upon a set of standards that stops code from varying drastically between devs.
- Readability: Ensures that the submitted code is not overly complex, that it can be easily read and understood by another dev if they have to make a change at a later time.
- Accidental errors: No matter how great of a dev we all make mistakes occasionally, just having a second set of eyes can greatly reduce those mistakes.
- Security: Critical security issues can be introduced in many ways especially by more junior devs, having a more experienced team member looking out for these could save you from a big data breach.
Think you’re ready to push your code?
Finishing a story can be a great feeling and it can be tempting to jump onto the next task. Taking a few minutes here can save you a lot of extra work if your code keeps getting sent back from reviews with reasons you could have easily made sure were sorted.
A quick list of things to do before you open that PR (may vary depending on your language and stack):
- Check the code compiles and runs.
- Check that your code fulfills the requirements of the story you’re working on. Make sure to check edge cases!
- Consider any regression testing you might need to do. Are you sure your change won’t break anything else?
- Read through your code changes and check for any syntax errors, workarounds you forgot to remove, un-needed code and limiting errors. If your project has a linter setup make sure to run that and fix any issues.
- Run unit tests!
- Check code test coverage.
- Update the story’s status and add any notes that are needed.
Things like unit tests, code coverage, and limiting checks can easily be set up to run as a git pre-commit/push hook.
Doing the review
There are too many tools out there for reviewing code for me to suggest a set way to do them. What you use will depend on your process, source control tools, and development pipeline but they should all let you do the basics you need.
As with the steps for getting ready for a review, these will depend on what kind of project and story you’re working on. A lot of the steps are the same as mentioned above as the committer should have already sorted any issues related to those… for these, the reviewer should just be confirming they are done.
Initial steps
Some of these could easily be automated into feature branch builds/hooks to reduce the time a reviewer has to spend on trivial tasks.
- Check the code compiles and runs.
- Check that your code fulfills the requirements story and any edge cases.
- Test the changes haven’t broken anything outside of the scope of the story.
- Read through your code changes and check for any obvious mistakes.
- Check there are no linting errors
- Check unit tests run
- Check code test coverage is above any team minimums.
- Check the story has been updated with any necessary information
Objective steps
This is the most important part of a CR where the reviewer can use their experience and domain knowledge to validate the changes. Things to consider when reviewing the change:
- Is the code in line with the team’s coding standards?
- Is the code readable and not unnecessarily complex?
- Could it be refactored to be more performant, maintainable or reusable?
- Could there be any security issues?
- Does the way the change was implemented fit with the larger architectural plan?
Rejecting
Make sure you are considerate when rejecting or commenting on a CR as it should not be viewed by the committer as a critique on them. A CR is a tool to improve the quality of the code being delivered and utilize a team effectively.
The use of reject should be used as little as possible in line with the team’s process. A PR should only be straight rejected if there is a major issue and should include a comment to explain why. Ideally, these should only happen if there is an issue that requires further investigation or a breakaway session to review with others. If the PR needs to be rejected due to unit tests or linting areas then the team’s process should be reviewed to add measures that prevent these from getting to a reviewer.
Comments
When adding comments, make use of the review tools to add the comments in line with the relevant code when possible. Make sure to keep the comments short and descriptive.
If something is not understood, try to have a chat with the person who committed it to avoid unnecessary back and forth.
Doing code reviews together
Having the person who did the changes sat with the reviewer can be very beneficial for all devs on larger or more complex stories.
Reviews like this can be especially useful for junior developers as it gives them a chance to get comfortable doing reviews while having a more senior team member to make sure nothing is missed.
Making time for doing reviews can be hard so it’s important to share the load between suitably experienced team members where possible. Don’t rely on a small set of developers to do all the reviews as this will get rid of any knowledge share benefits.
How code reviews fit into your process
To make code reviews effective you want to be doing them before the code goes into a shared branch so the review gets done on a feature branch. This ensures any issues in the change are caught before they affect other developers or end up in an environment.
How to add code reviews into a development pipeline is a little outside the scope of this post so the actual implementation is up to you.
