When we started with the MSNBC project, my colleague, Jerad Bitner, established a process that each ticket would be implemented in a Git branch and a pull request would then be created for someone on the team to review. I had done a bit of peer reviewing in the past, but this experience was totally different.
In GitHub, when you are added to a repository, you get notifications for all issues and pull requests that are created, unless you change a setting. I don’t know why, but every morning while catching up on email, I started looking at all of them. I realized that apart from learning from everyone’s code and discussions I was gaining a feeling of safety by creating a mental picture of the site. This gave me a tremendous amount of confidence to chime in on pull requests, suggesting small improvements that helped with the quality and consistency of the entire codebase.
Even in times when there was a lot to do, I kept looking at pull requests. I had to. I felt that if I stopped doing it I would end up duplicating code or my code wouldn't follow the same approach taken in other areas of the codebase. In this article we will see tips and examples on how to do peer reviews within a team.
Improving code together
Peer reviewing has become a habit for me and GitHub’s pull requests made this process easy and fun. They make the code visible for everyone to discuss it and they are a great chance to sneak in small improvements in the surrounding code like in the following example:
The code above is correct and meets the requirements of the related ticket but here we are suggesting using a function from our codebase that achieves the same in a simpler way. Yay for standardization!
Keeping the pace while reviewing
Discussions on a peer review must be as efficient as possible or they would slow down progress. As soon as we see that we are taking too long in a discussion we would create a follow up ticket and merge what was implemented in that pull request. Here is an example:
By creating follow ups we create an opportunity to rethink what was discussed and create a new pull request to test it and potentially merge the improvement. It keeps both project managers and developers happy as the project keeps evolving at the right pace.
Tone to use in peer reviews
The tone is very important in peer reviews. Being positive, constructive and vulnerable are key attributes to connect with someone’s work. Here is an example of typical suggestions and their feedback:
Note the use of words like please or could. At the end, we are just making suggestions for the code to improve. Even when we find bugs in the code or missing functionality, keeping a constructive tone in our comments encourages team members to improve instead of provoking a defensive response. Some Open Source projects like Drupal reflect how its members should communicate in a Code of Conduct.
There is always time for a laugh
There are times when making a joke of how we feel about something may be the key to break the pressure of a deadline and inject good energy. Note that we should take a lot of care on when to do it and whether the author of the pull request has a sense of humor that connect with ours. Here is an example where it worked great:
Responsibility is shared among the team
By contributing to someone’s pull request with a peer review the team gains a feeling of shared responsibility over the code. It will be much harder for a bug to sneak in if there are more eyes looking at the code before it is merged into the main branch. Moreover, if there is an issue with the code there will be two or three people who can look into it and they will back each other up to fix it. Here is an example of a bug caught by peer review that never made it to production:
Not everyone likes to peer review though and this is fine. You can’t expect the whole team to review everyone else’s pull requests. However, having at least a couple people doing it makes a big difference. It does not need to be the senior back end and front end developers: less experienced folks will ask different questions which are valuable feedback too.
Getting other roles involved
In order to get project managers, external teams and the client into peer reviewing, we integrate the GitHub repository with Jenkins to spin up a testing environment for each pull request. We can see this in action in the following screenshot where we not only get a link to the testing environment but we also see results of end to end tests:
This tool affords the opportunity to test something before it gets merged into the main branch for people who do not have a development environment. It saves us a lot of time to obtain feedback.
Making code easier to read
Peer reviewing starts by reading code that someone has written. If the code is easy to read, then we can go to the next step (testing it). If the code can’t be read easily, then we ask questions. A few days ago I read this quote from the book The Pragmatic Programmer:
Remember that you (and others after you) will be reading the code many hundreds of times, but only writing it a few times.
This quote really struck a chord with me. Our code will be read by many others (even ourselves) and therefore it has to be as clear as possible. The term clear has to be agreed within the team. Personally, I follow these steps before I ask for a peer review:
- I check that the code is clear and meets our coding standards.
- I check that I have commented my code enough and that comments are well written.
- I check that the commit messages match with the changes in code.
- Finally, I create a pull request and add testing instructions at the top for someone else to peer review it.
The main point of the above list is that I want my code to be tested and merged quickly. Therefore, the easier I make it for the peer reviewer, the better chance I have to get it into the main branch and resolve the related ticket.
It’s a learning experience for everyone
Being vulnerable while peer reviewing has the benefit of learning. I have learnt a lot of things while doing peer reviews just because I asked about a particular line. For example, I had no idea of what $(function () was for in JavaScript. This is how I learned about it:
And there is more: when I was convinced that the function empty() was bulletproof, I stumbled upon this:
The peer review checklist
Here are the guidelines that we used at the MSNBC team. If you want to try peer reviews in your team, you should get together and agree on a list of steps like the following ones:
- Does the code follow our Coding Standards?
- Is the code well documented?
- Are there testing instructions on the ticket?
- Does the code rely on our existing APIs?
- Are all comments in the pull request resolved or discussed?
- Does this pull request need any follow up tickets? Are they created?
- If there is a testing environment, do all tests pass?
- Are the requirements of the ticket met in your local environment or in a testing environment?
- Are there database updates? Do they complete without any errors/warnings?
- Are there any changes in Features components? Do they revert as expected?
- Look back at the requirements — are they met in an efficient manner? Would you have architected the changes differently?
Conclusion
My aim with this article was to share how beneficial it has been for me to do peer reviews. Do you think that this applies to your project? Try it out with your team for a couple weeks and share your thoughts here with us. I am sure that you will get something good out of it.
I take the chance to thank my colleague, Jerad Bitner for getting the process into the team and James Sansbury for teaching me a lot about tone and thoroughness. Also, Matt Oliveira and Sean Lange provided great feedback while reviewing this article. Thanks also to all the bots who participated in the BoF about peer reviewing at the Lullabot retreat.