Code Review That Helps
Having an efficient code review process is essential to ensure code quality, grow as a team and get a good grasp of the entire project. However, different people view code review in different ways that it either sounds boring or exciting.
In this article, I would discuss ways to conduct good code reviews and make it a means of learning/adding value rather than a political or emotional argument.
Code review (sometimes referred to as peer review) is a software quality assurance process in which team members go through a computer program mainly by viewing and reading parts of its source code to spot mistakes.
Before a code review process can begin, a pull request has to be made. A pull request (PR) is a request to introduce changes to a code repository through a Git branch. This is done using tools like GitHub, Bitbucket, Gitlab etc. Once the pull request is opened, the Engineer notifies reviewers to give their feedback on code improvements (and updates the code if necessary). When the Engineer and reviewers are satisfied with the code, the changes are merged into the base branch.
Why code review matters
Code review is essential to ship quality software solutions to real-life problems rather than introducing one. Despite the experience level, it is necessary to have someone review your code. It
- ensures that coding standards are adhered to
- facilitate knowledge sharing
- helps team members make an extra effort to ensure code quality
- helps everyone get familiar with the codebase and best practices
- provides room for someone else to suggest a better approach to solve a problem
- limits the risk of introducing bugs thereby improving code quality
Questions to ask yourself during code reviews
When conducting a code review, ponder on these below. Answering each of them helps you know things to look out for during the review process.
- Is the piece of code difficult to understand? It might need refactoring
- Are the method and class names intuitive? They may need to be renamed
- Does the method do much? It may need to be broken down
- Are there many comment blocks? The method names may need to show what the code does
- Is the code well organized? It may not be maintainable
- Does the code have test coverage?
- Could the code be made more efficient?
- Are all the requirements, stated in the ticket fully implemented?
- Does it meet the coding standards of the team?
How to do a proper code review
Reviewing someone's code should not be about the approvals. It is about helping a co-worker find mistakes in his/her code and ways to make it better. When someone sends you their PR to review, it means that the person values your time and input. Find time to help and do it properly.
Look out for deviation from coding standards, readability, maintainability of every piece of code, test coverage, performance, accessibility and security bottlenecks.
Review your code
It is important to review your code to spot mistakes before requesting for a code review. Ensure that your code compiles successfully and passes the test cases. This helps you spot little mistakes before your reviewers do.
Foster a positive culture
When a comment is made regarding a part of the code you didn't write, don't take offence; rather be happy to fix it and help in paying technical debts. Avoid words such as "That isn't my code. My linting tool only formatted it". Have a positive culture around code reviews, it should not matter who introduced the bug. All that should matter is preventing the bug from being deployed to production. Or, eliminating the bug if it wasn't caught earlier.
Automate what can be automated. Make use of stylelint, eslint and other tools to help improve the quality of your code. This helps code reviewers focus on the code and not spend time spotting issues that can be found automatically.
Let one PR do one thing. When the purpose of the code change is clear, reviewing becomes much easier and increases feedback.
Have a context of the problem
Your job is not just to spot syntax errors, having a context of the problem is important to create a bigger picture and be sure the solution fits into it. To help team members get an insight into the problem, it is paramount for the submitter to link the issue / ticket number to the pull request. If it needs further clarifications, provide it in the description field.
TicketNumber: Issue description
Fixes #issueNumber: Issue description
Note that when constructing the commit message, start with a verb e.g. SPA-001: Bootstrap the app
Attend to comments
You certainly should not leave a comment unattended, you either respond to it or fix the issue. If you do not understand what the reviewer meant, ask for clarifications; don't be emotionally attached to your code. Afterwards, notify the team member that dropped the comment that it has been attended to. Going ahead to resolve a comment dropped by another or merging a PR with unattended comments is a way of telling the team member who dropped the comment that you do not value his/her input.
Rephrase your objection as a question. It helps to get the desired result without much argument.
Bad: “This change will break feature XXX.”
Good: “Will there be a possible negative impact on XXX with your change?”
Be open to new ideas
You may have an entirely different approach to solving a specific problem. Being open helps you learn new and possibly better approaches instead of imposing your approach on your team members.
Bad: “This approach sucks, use XXX.”
Good: “I have a similar feature implemented using approach XXX because YYY does ZZZ. You can learn more about this on LINK XXX.
Acknowledge people's level of experience
Everybody is not on the same level of experience with you, so state the obvious without being sarcastic about it or treat people like they don't deserve to be Software Engineers. Pointing out useful examples or suggesting a way to implement it will help get everyone on the same page.
Bad: “Can’t you see that this is wrong, letting a break in XXX break the app?”
Good: “You can make this fail-safe and return early by doing XXX.
It doesn’t cost anything extra to be respectful when letting people know that the submission doesn’t meet the code standard for quality.
Bad: “This code doesn't meet the minimum standard. Didn't you read CONTRIBUTING.md before making this PR?
Good: “Thanks for your contribution. However, it cannot be accepted in its current form; there are multiple problems (as outlined below). Consider reading CONTRIBUTING.md, that could help us find a path forward.”
Be a team player
When reviewing many lines of code at once, you are less likely to find bugs. So, if a PR contains many files that will take much time to review at once, be polite when letting the submitter know about it. The codebase belongs to the team. Also, as a submitter, keep your pull requests small to speed up review time.
Bad: “This contains quite a lot. I do not have time to review it.”
Good: “Could you please break this down into smaller changes? I do not have a lot of time and this contains quite a lot to review at once.”
When a PR wouldn't be allowed to slide through due to details that may seem minor like formatting, saying “please” shows that you respect the submitter’s time and can go a long way in getting the submitter to make the change without feeling bad.
Bad: "This code is not properly formatted"
Good: "Could you please align these variable definitions so they’re easier to read?"
Always show appreciation when a team member does a good job. As a code submitter, also show that you value feedback from the reviewers. Showing that you appreciate as a motivation.
Peradventure code review is not yet a part of the code shipping process in your organization, you can introduce it and watch the effect it will have on your team. However, if the code review process is not well planned, it could have more cost than value. Enjoy the process, do not see it as a waste of time.