Code Review Etiquette
Overview
Code review is a pivotal part of the Software Development Life Cycle. As a team of engineers it is very important to have thorough and helpful reviews done within your team. I remember on the first team that I was on that started implementing Pull Requests and Code Reviews it was met with a lot of resilience. The team didn’t think they needed their code reviewed at all. The team I worked for was very proud of their code, but they were also very siloed. No one knew how others code ran or how it worked. Which made us all hate when anyone else left on a vacation and we had to watch their services.
Those that embraced the change really grew from having others give their input. They were able to more immediately have better code and less problems in production. It also helped build comradery by starting good conversations around what are the preferences of the people on the team.
For those that resisted this change it turned into a lot of sneaking through changes and talking bad about the system under their breath in meetings. They would close out comments without making changes or saying anything and it eventually made those of us that were enthusiastic not want to review their code. I saw these people end up getting even more siloed in their projects and then getting overworked as they made more and more decisions by themselves. These people ended up leaving our company and it was better for everyone. Except for us inheriting systems we basically knew nothing about.
Why Code Reviews Matter
I believe that the code that is being written is not only owned by the engineer who wrote the code, but all of those who work on the project. All code that goes into production will rather directly or indirectly affect everyone on the team (and most likely the business as a whole). With that in mind you should want to push up the very best code that you possibly can. Code Reviewers should feel enabled to give thorough and honest code reviews. By having quality Code Reviews the engineers will learn together and grow. Also helping to build good communication skills.
Pull Request Requirements
Within the teams that I run I have specific requirements that have to be met before a Pull Request can be merged. These requirements can change based on the needs and personnel of the team. These rules are the basic rules that most teams I have been on follow.
| Requirement | Description |
|---|---|
| 2 approvals of the most recent version commited | The PR should have 2 approvals based on the most recent committed version. If you had 2 reviews and the PR was sent back for more review after QA, the PR needs to be re-reviewed. |
| 1 approval from a senior level engineer | 1 of the 2 approvals needs to be a senior engineer or higher. |
| All checks passing | Github runs several checks depending on the repo. All of those checks should be passing before merging. Some of the following checks might be linter passing, tests passing, build process successful, load testing or a package vulnerability tester. |
Etiquette of Code Reviews
Creating a Pull Request can be a vulnerable moment for engineers as they show the work they have been working on. To make it a better experience for both the reviewer and the reviewee, you should try and follow these etiquette principles to allow you to focus on the coding and not on hurting feelings or making it personal. These will help those you work with be more receptive and appreciate the work you are doing as a reviewer.
1. As the reviewee, I should respond to all code reviews.
Someone has taken the time to review your code. You should acknowledge all responses and rather make a change or give reason as to why you are closing the request.
2. As the reviewer you should keep the reviews objective.
Often times we have opinions about the readability, nesting, testing and so on. We should have a strict set of written standards (I’ll be posting an example of my PHP standards for some of teams and how we came up with them soon) and coding principles we follow that are established before code even is written. Opinions outside of the followed standards should be brought up to be put on the standards.
3. Respond to Pull Requests within 24 hours
No one likes to wait around for their work to be reviewed and stale code benefits no one. We should be on top of the Jira boards and know when there are pull requests that need to be reviewed.
4. Not all feedback on reviews needs to be criticism, positive feedback should be encouraged
It feels good to be told by your peers that you are doing something well. If you are reviewing a PR and have left a couple items to be fixed, throw in something that they are doing well. That type of acknowledgement helps people to accept the items needing improvement much better.
5. Everyone reviews Everyone’s code
Some people I have worked with or managed have come in with the idea that you only review those who you have seniority over. I have heard you wait til you have a certain amount of knowledge about the system or time with the company. I have a rule that I give on all of my 30/60/90 expectations for new employees that you should review and comment on each of the other engineers code on your team within the first 60 days. So if you are Junior, I expect you to criticize my code, the seniors code, whoevers code goes up. Everyone should have a voice and they should learn that quick.
Summary
Everyone talks about having a culture where you “Leave the Egos as the Door”. Code Reviews to me are where you put your money where your mouth is and see where the egos on your team are at. The more honest and clear you can be in your reviews and receive them, the better the place you are at.