If you do code review, you likely come across the same issues over and over again. People who are contributing to your code base for the first time may not know your team's internal standards, they may come from a different language and do things slightly differently. A new hire may need to get up to speed quickly on submitting changes to your repository. How can you make sure these cases are handled while minimizing the impact on your code reviewers? One helpful tool is the "Code Review Comments" document.
A "Code Review Comments" document is a collection of comments that come up again and again in code review sessions. For example, if you're always having to tell people "Make sure you're using prepared statements when sending input to an SQL server", you could just add that to this document, and then when you see unescaped SQL, just point the submitter to your doc.
Onboarding New Developers
When a new developer joins your team, you can point them to this document so they can get a quick overview of your standard practices. This is a good place to put things like:
- All code must be linted, and conform to our internal code style standards before being submitted for review (you do have code standards right?)
- Code submitted as a PR should be tested by another developer before it's merged
- We prefer using "guard clauses" rather than deeply nested if / else blocks see: https://www.thechrisoshow.com/2009/02/16/using-guard-clauses-in-your-ruby-code/
Anything that's not covered by your linter/code formatter can live in this document.
Decreasing Cognitive Load
Reading code is generally considered harder than writing code so anything that places less cognitive load on the reviewer is good. If all the simple stuff is covered in your Code Review Comments document, then the reviewer can focus on the meat of the change request without having to worry about stuff that they shouldn't have to worry about. This will also help with personal style differences that may come up in a review. If it's clearly stated that your team prefers shorter variable names, then it's harder for someone to argue that in this case, they really think that loopIndexVariable is a better name than i for a loop index because it's more descriptive.
Limiting Bike Shedding
If you find yourself constantly getting into arguments over stuff that's really not that important, you can throw it in the doc (once the team comes to consensus on the best colour to paint the bike shed) and then any time someone thinks another colour would be better, just link them to the doc and be done with it. For more on Bike Shedding.
Formalizing Team Standards
Often, the longer a team works together, the more unwritten rules they will develop for how they like their code. This works great until you try to bring a new person into the group. The new person needs to discover these standards by trial and error, which can be frustrating and take time. Maintaining a Code Review Comments document can help formalize these unspoken standards and reduce friction when a new member joins the team. Instead of having to have several code reviews rejected because of internal standards that everyone else is aware of, the new developer can just read the document.
Don't Over Do It
One thing you definitely don't want to do with this type of document is to fill it with so much minutia that it itself becomes a source of cognitive load for the developer. It should be fairly succinct, and easy to read. A good example of this is the go teams Code Review Comments document. It's filled with things that have come up over and over again in code reviews, and have been added to the document to improve the development and code review experience for both the submitter and the reviewer.
Starting Your Own
It's really easy to start a document like this. Any time you're doing a code review and have to comment on the same thing more than once, add it to the document. Get your team to do the same, then you can review the doc together from time to time and decide what to keep and what to discard. When new contributors join the team, send them the doc before they submit their first PR so they can make sure their code is ready for code review.
Conclusion
Maintaining a Code Review Comments document can be a good way to onboard new developers, decrease the cognitive load on your reviewers, and formalize unwritten team standards. Instead of having to discover these things via trial and error, a new contributor can scan the document and get up to speed with the internal standards of a new team quickly. This can reduce friction for the new developer and improve the code review experience for all parties.
Do you use a document like this with your team? Feedback, thoughts? Leave a comment.
Top comments (5)
Great article! I've been keeping a list of the comments made on pull requests I've opened and have encouraged my coworkers to do the same but I really like the idea of a shared document for the team to reference.
Hey Gabriel, great article. I'm curious about the following statement:
How do you do that in practice?
Hey Sten, bit of a typo on my part, it should read "Code submitted as a PR should be tested by another developer before it's merged"
I'll update the article.
Ah thanks, I thought you were doing some Gerrit-style magic 😅
I've used phabricator in the past to do "non pr" diffs...but yeah, in this case just a typo!