Daniel's Blog
Reviewing Code Reviews
3 min read | Last Updated: July 14, 2020
Once I was in a team where we struggled with code reviews. We got along really well and delivered a lot, but that was the one thing not working as well as it could. Some of my teammates were meticulous while we were of the “done is better than perfect” way. All of us were right and wrong – We wanted to deliver quickly with a “good enough” quality while they preferred to have very clean code, easy to understand and have the same feel, even if delivery was a bit slower.
In the end, we agreed that it wasn’t working as it should and we sat down to decide on an approach that would work for everyone. The goals were – ship quality code, accept different styles and listen to others’ opinions. To achieve this, we came up with four types of review comments.
Side note: Back then we went with “i.”, “q.”, “s.” and “c.”, but use whatever feels right here.
The four types of review comments
Issue: Use this cautiously, because there is a problem in the PR that needs addressing before deploying. “Solved” here can mean a conversation, code changes or a new tech debt ticket.
Question: Questions can be leading like “Will this crash if X happens?” or more questioning “Can you explain why this module does, I don’t know what this service does?”. You should get answers to them before the PR gets deployed, and some of them might turn into Issues or Suggestions.
Suggestion: Use suggestions when you would write some code differently, or with some styles that don’t affect production or the consistency of the codebase. For example, I prefer using comprehension lists over pure loops, so sometimes I might pop a suggestion “This could be instead [one liner]”.
The critical bit on suggestions is distinguishing between a design decision that can affect the consistency of the codebase (i.e. we use trailing commas everywhere but in this function) vs putting your flavour (i.e. loops vs comprehension lists). If the code gets more confusing as a result, an issue could be appropriate as well.
Another great thing about them is that it reduces the severity of the feedback. People tend to be less defensive when they realise you are genuinely trying to help them. That said, just because the suggestions are not mandatory, doesn’t mean they can be ignored. Work together to understand what you are trying to do; perhaps you can learn something new today!
Comment: It’s free text, put whatever you want in here.
Don’t add process for the sake of process
Use this if problems with reviews that are due to different styles or coding standard expectations are starting to pop up. Expectation mismatch often happens in recently formed teams, where the communication flows are not fully defined.
If there are attitude problems on the team, with colleagues being dismissive or rude on reviews, address those as management issues without using this method.
My final recommendation, which will sound counterintuitive, is that you should not be using this for any extended period. These guidelines are a crutch to help teams [re]set their expectations on code reviews, and especially useful if they can’t easily do pair reviews in the same office. Most teams do this informally to some degree.
Happy reviewing!
Written by Daniel Lopez Rovira who likes talking about engineering stuff.