In the first part of this series, we’ve taken a look at an introduction of Google’s code review best practices, using the “Standard of Code Review” section in the reviewer part to relate to both the reviewer and the change author. In this section, we will take a deeper dive into the code reviewer’s aspect in this, still taking in Google’s views and also adding some of my own opinions along the way.
What to look for in a code review
These are the bullet points to this section which is copied from the end of the page. To see a more detailed summary of this section, click here for the original or here for my summary of it.
In doing a code review, we should make sure that:
- The code is well-designed.
- The functionality is good for the users of the code.
- Any UI changes are sensible and look good.
- Any parallel programming is done safely.
- The code isn’t more complex than it needs to be.
- The developer isn’t implementing things they might need in the future but don’t know they need now.
- Code has appropriate unit tests.
- Tests are well-designed.
- The developer used clear names for everything.
- Comments are clear and useful, and mostly explain why instead of what.
- Code is appropriately documented.
- The code conforms to our style guides.
Navigating a CL in review
This section is divided into three main steps which would best be followed in order. The first of which is comparing the content of the change to what the change should contain, given the context of what is happening in that current sprint. Does it match what should be delivered in the task given? If not, we should provide as much detail as possible as to what was required and how what is currently in the change list does not match.
Communicating this, and communicating in general, can be difficult for devs in this situation. And so sometimes we get a situation where we avoid the code review, or it’s rejected with comments that could rub the change author the wrong way. Both approaches causing negative situations to be solved later on, something which could have been avoided by speaking more politely and positively.
Examples of how to communicate this rejection can differ for companies of different cultures/languages, a general sample approach given by google is as follows:
“Looks like you put some good work into this, thanks! However, we’re actually going in the direction of removing the FooWidget system that you’re modifying here, and so we don’t want to make any new modifications to it right now. How about instead you refactor our new BarWidget class?”
The second step relates to what part of the change list we should look at first, called the “main” bit. The definition of this is a file that has the most logical changes. In larger change lists, this file is most likely the one that is forcing/causing the changes to occur in the other smaller files.
Using this approach increases the speed and accuracy of reviews and gives an overall capture of the context of what is happening as a whole early on. Noticing things that can be improved in this part of the CL should be commented on and made notice of as soon as possible. Two main reasons for this: sometimes devs might start other work based on top of this CL (which, in reality should be avoided) and letting them know early on would halt them from doing that. Letting them know early on also helps when there are deadlines involved, the quicker the notice the quicker the change the more chance the deadline task is done before time.
The third step is to look at the change list in order, this ties in with the previous step, as the first change is usually where the biggest change is. Running through it like a story helps with understanding the process better, and hopefully helps in offering suggestions for improvements.
Speed of Code Reviews
Why Should Code Reviews Be Fast?
This subsection of the section of the Speed of Code Reviews page explains the reasons why Google believes that code reviews should be conducted in a fast manner. These are the consequences that are likely to occur when code reviews are slow.
- The velocity of the team as a whole is decreased. (Speaks for itself).
- Developers start to protest the code review process.
- Nobody wants to wait on our approval for a long time when we asked for a major change of a change list, it’s demoralizing and discourages pro-activeness.
- Code health can be impacted.
- As mentioned in the previous bullet point, if people are discouraged from making positive changes because they know it will take a long time to accept, then the overall health of the code base suffers over time.
How fast should code reviews be?
- “you should do a code review shortly after it comes in”
- “One business day is the maximum time it should take to respond to a code review request (i.e. first thing the next morning).”
- “a typical CL should get multiple rounds of review (if needed) within a single day.”
Speed vs. Interruption, Fast Responses and Cross-Time-Zone Reviews
If we have work that we are “in the zone” on, it would be pretty difficult to get back on it if we get off now to do a review. Instead, this is where the personal velocity trumps the team velocity. It’s probably best for the team velocity for us to finish what we are doing first, and then find time in between where we feel our creativity won’t be disrupted. Pairing with someone early on before they even create the pull request would help solve a lot of this and provide us with more opportunities to review with speed.
This part sums up perfectly why we should try and provide fast responses in code reviews:
“Even if it sometimes takes a long time to get through the entire review process, having quick responses from the reviewer throughout the process significantly eases the frustration developers can feel with “slow” code reviews.”
Time zone differences can also play a part in the speed of responses or reviews, ensuring that our responses occur during the same time they are at work should also be considered.
LGTM With Comments (Looks Good To Me)
Providing LGTM comments can mean one of two things:
- I’m confident that the remaining comments will be resolved/addressed with ease
- The suggested changes are optional and can be omitted from the change list if desired
It could be a good idea to try and be clear about which of these two LGTMs are intended for that CL, in order to avoid confusion.
Large CLs
Is the change list so large that it can make it difficult for us to read/review all of it without wasting a huge amount of our time? Is there a chance that there is more than one major change in the CL? If so, can one major change in this CL survive without the other ones? Maybe we could split this pull request into smaller ones. If not, we can comment on the design a whole, what we have reviewed and how far we’ve gotten, to give an idea of the progress of or review.
Code Review Improvements Over Time and Emergencies
Practicing these should gradually improve the speed of code reviews over time.
Emergency situations are one of the only types of situations where code reviews should go through the whole process very quickly. However, we must be able to define what emergencies are in order to treat the code reviews as such:
- Allows a major launch to continue instead of rolling back
- Fixes a bug significantly affecting users in production.
- Handles a pressing legal issue.
- Closes a major security hole.
- Ensures we don’t miss a hard deadline.
How to write code review comments
This page gives guidance on how to approach providing comments/suggestions to the change author. These approaches should be able to bypass a lot of the personality/experience matrix of scenarios that we might encounter in these situations.
The summary here is copied straight from the source, with one or two added in:
- Be kind and respectful and Keep code author’s confidence in mind (experience/personality dependent?).
- Explain your reasoning and allow them to explain their code.
- Balance giving explicit directions with just pointing out problems and letting the developer decide.
- Encourage developers to simplify code or add code comments instead of just explaining the complexity to you.
- Not just commenting on what needs to change, but on what they also did well.
- Try and gauge if the receiver appreciates compliments (chemistry/opinions varies).
Handling push-back in code reviews
In some cases, there can be disagreements on the code reviews/ comments we place in the pull request. That is totally normal. It can be generally difficult to take personal opinions and egos out of a code review, sticking these summarized subsections should help keeping things in perspective.
Who is right?
This is slightly related with what to look for in a code review, with regards to the issue of the context and how close one is to the code most likely to be the change author: in which some cases, the change author would be correct and can justify by giving detailed and appropriate evidence/reasons. However, in some cases if we truly believe we are correct, and feel that our suggested change would result in better code health then we should not shy away from probably going “a few rounds” before we are finally able to convince them.
Upsetting Developers
Disagreements can cause upset or a feeling of being annoyed from the change author. This feeling, however, would most likely subside and end up in the change author being thankful for the change later. Some upsets can be justified, as sometimes it’s not what we say but rather how we say it, so we need to keep our approach in mind.
I’ll clean it up later
In a code review, we might receive this comment from the change author in order for them to be able to pass up the pull request quicker. Some devs will truly get the suggested changes done directly after the first pull request is complete, and some forget it among the mountain of things they must do. It happens. So as a result, we must try and encourage the code author to strike at the moment while the iron is hot.
There are some circumstances in which another pull request to clean up is preferred, this is when the suggested change could cause problems surrounding the code which cannot be addressed at this moment. Hence, it is mostly preferable at this point to raise a bug or create a TODO comment in order to clean up this code in another pull request.
As we sum up the reviewer side of Google's best practices, we take into account the journey the reviewer goes through and what they would look out for. While navigating, the reviewer must also take consideration of their approach to comments, the speed in which they reply and how to handle disagreements between the two parties. In the next post we will be looking into the change author's side, and how they can improve the overall standard of the code review.
Thank you for reading! 😃
Top comments (0)