Yet Another Code Review Best Practices
During my developer’s career, I was a member of several different teams. As every team has its own process, way of working and arrangements, introducing Code Review was always unique experience. However, in most cases, the greatest part of discussion and meetings were related to technical aspects such as tool or service, which will be used to do CR and its configuration. I agree that keeping process lightweight is important, but that’s not a key of Code Review.
One can ask why we need to think of CR practices at all. Review of code written by another team member can be done without any preparation or introduction. It is true, but there are some aspects that we need to think of. In my opinion, we especially need to support the following values:
- Quality – Code Review should be done in the same way, no matter if you are hurrying, bored or it’s your 3rd hour of doing CR,
- Motivation – team has to see value in Code Review, and this value shouldn’t be lost because of negative comments or “Big Brother” effect.
Below, you can find practices that help me during Code Review regardless of a project I am working on.
1. Have a goal
It is important to have a list of common goals, which every member of a team believes in. Remember that goals depend on project and team structure, they will differ across teams. It is important to be open minded while discussing and selecting goals for the whole group. Here are a few examples:
- Eliminating bugs at early stage
- Improving code quality
- Increasing knowledge about the project
- Learning from each other
2. Create your own checklist
Think of what you need to pay attention to when reviewing code. In most cases, such a list is a personal one and can be used across different projects. It’s good to compare your list to the other ones, so all necessary items are included. Keep in mind that you need to check not only what is written in code, but also what may be missing.
Automate as much as it is possible. In most cases, style checking can be fully automated. Counting a number of spaces or blank lines does not help in bringing business value, so when you find style issue, you should review project’s static code analysis tools and its configuration. It is really worth writing your own custom rules, if the rules provided do not meet your expectations. Of course, in case you add a new rule, check if it does not conflict with any other enabled one.
4. Think of change aim
Most of changes are introduced in order to put some business value, let’s say by implementing a user story. Before reviewing, read requirements and try to find references to story in the code. Every moment is good to check if story meets expectations, including Code Review. Also, the change should be consistent, it means that it shouldn’t include any code that is unrelated to the user story.
5. Look for tests
Check if there are automated tests created for a given piece of code. If you think that it is applicable, and tests are not yet created, offer to do so. It will improve code quality. In addition, tests can help to understand the code. If they are present, you should begin with reviewing them.
6. When commenting
- Try to check the code as soon as changes are available in the latest version. Otherwise, make sure that the code is still present in the latest version. If not, do not comment. The author won’t be happy to answer your remark when there is nothing to be done. That’s an extra, unnecessary work.
- When an issue is found, try to reproduce it. It will make you sure that you are right and help to provide a better feedback. Additionally, it will improve your understanding of code and project knowledge, because you see how application behave just looking at code.
- Find out whether your comment meets any goal from point 1. Comment that doesn’t provide any value is really demotivating for the author. Consider whether your change proposition really does matter. The authors’ code may differ from each other, of course if both meet style standard.
- There will be cases in which you find a code that should be fixed, but this part of code is not included in the change, it’s in the same file. Consider whether commenting will bring any value. If not, you can fix it by yourself, as a whole team is responsible for code quality.
- Try to understand why such a solution has been applied instead of writing that the code is wrong. You don’t have to be always right, and a comment should be a beginning of discussion, not just a change. Try to coach the author.
- If you think that some changes are needed, provide full solution. It won’t be fast and easy. Big changes, such as design enhancements, may require to analyze larger part of code than what have been committed. In such case CR tool won’t be enough. You will need to use IDE in order to access full code t but it will confirm your thinking. Your solution is a good base for further discussion, and helps the author understand your intentions.
- You don’t have to always use CR service for discussion . The author of the code isn’t another user of CR tool, he’s another team member. Therefore, don’t hesitate, just present your doubts directly to him. Sometimes, it is better and faster solution.
7. Positive feedback
When you find some inspiring piece of code, share it with the team. It will motivate an author and may be useful for the rest. When it comes to negative feedback, it’s just the opposite – don’t escalate it. No one likes when every single remark is spreaded.
8. Review yourself
Before committing code, review it just like you review any other code. You may be surprised, but it looks differently when you see only changes that are pushed to source code repository. It is faster to review your own code than the others’. When someone finds an issue that’s on your checklist, you won’t feel good.
9. Code means everything
Review every change that you are able to read, no matter if that’s an SQL query, shell script or configuration file. There are cases in which even graphic and automatically generated files should be reviewed. Also, remember to review internal tools and tests.
10. You don’t have to be an expert
It’s a great chance to learn something new. If you are not sure whether solution is correct, you can start a valuable discussion. As a team member you should be able to work on any area of the project. Don’t allow any person to be the only expert in specific part of an application. You should be able to replace anyone in case of his absence.
11. Maintain process
You don’t have to follow all the assumptions that were set when Code Review process started. If you feel that something should be changed, just change it. When you find an issue that’s not on your checklist, think if it can be generalized and added to your list.
Tips that are provided above won’t work in every project, but it’s always good to confront your current practices in order to see if Code Review can be improved. Process should be continuously enhanced . In order to find inspirations for new improvements it may be good to see how it works elsewhere.
A few examples of tools which may help to automate analysis:
Is there something missing? Do you disagree with any point here? Or have you learned something?
Don’t hesitate to comment!