Looking at Google’s code review best practices, we have so far summarized the overall standards of conducting code reviews (link here) and looked into the code reviewer’s side (link here). In this latest instalment we will be giving a summary on the change author’s side and the actions they can take to make the experience of code reviews better/smoother for the code reviewer. This will also include suggestions from my side as a mix to what is being given from Google's side.
Writing change list descriptions
The first line counts
Some organisations have notifications that pop up in the slack channels whenever a pull request comes up. The first line, if not the header (which can be considered the first line I guess), of a pull request almost always gets taken as part of the notification; this is what the potential code reviewer first sees. In the chance that they might not know the full context, but still want to help out, the first line can always help set them in the right first step to understand the context of the change list; therefore, spend less time trying to understand it when in code.
Google's Engineering Practices documentation has kindly summarized the attributes that make a first line great.
- Short summary of what is being done.
- Complete sentence, written as though it was an order.
- Follow by empty line.
If possible, we can try to include a specific word that we feel may be the most searched for in the future by someone looking for a particular change to do with our code.
Body is informative
The body of the description is usually where we would describe all that occurred in the change list, normal sentence or bullet points (whichever is felt more efficient to the team). Including a brief overview of the problem being solved at the start would help in grasping the context of the CL. Some organisations use devops tools like Azure Devops which already include linked tickets/tasks to the pull requests.
In order to help the code reviewer, navigate the code review, it could also be beneficial to structure the description in a “template-able” format (if that’s a word). That way, as the code reviews roll out, the brain is already trained to look in the right places and get the information it needs to quickly grasp context.
A probable structure to a CL description body could be:
- Brief overview of problem being solved (optional/depending on devops tool used at time)
- Main places changed (brief desc of reason for change)
- If including links, add extract in case the link becomes unavailable in the future
- Minor changes
- Any concerns about approach taken (could be very important)
Bad CL Descriptions
This one speaks for itself, here are some examples being shared that provide no context and could possibly confuse the code reviewer even more.
- “Fix build.”
- “Add patch.”
- “Moving code from A to B.”
- “Phase 1.”
- “Add convenience functions.”
- “kill weird URLs.”
Review the description before submitting the CL
Pull requests can change during the time that it was made, so its always important to keep it accurate to the last commit.
Small change lists
As we go on throughout our careers, we come across more and more pull requests that we feel could have been separated into different ones. There are many advantages to keeping change lists very small, which is shown in the main page in a list (link here)
What is a small change exactly? We can tell if something is small enough when the change addresses an issue in its entirety and most likely doesn’t affect a large number of files. It’s usually one thing that is changed and not many files were affected to solve the problem, if that occurs then that could mean it’s a complex solution for a simple problem. The rest of the list of what a small or large Cl looks like can be found here.
Separate Out Refactorings
This, for me, can never be stressed enough especially if the refactoring is large enough to warrant its own separate task and pull request. We can usually tell this if the context of what was supposed to be solved is partly taken over by the refactoring and we spend some mental real estate thinking about the refactoring rather than on the intended change list.
Very small changes/refactorings are alright and shouldn’t be looked at too strictly. Read the room or code in this case 🔎
Keep related test code in the same CL
Does what it says on the tin, any change made to production code should generally be reflected in the new test cases to be added as part of the change list submitted. Refactoring tests or introducing a new test framework (refactoring a few files as a result) is advised to be separated into a later change list. It’s also worth noting that the code should tested locally, and of course builds should not break, before any change list is created/committed.
Can’t Make it Small Enough
Ego can get in the way here when we create pull request for a very small change, some of us may feel we need the PR to be larger. The opposite is the case, the smaller the easier it is to commit and be confident in.
How to handle reviewer comments
Don’t Take it Personally
As mentioned in the previous instalment Part 1: Two sides of the same coin, the main aim in a code review for both parties (CL author and code reviewer) are to improve the overall health of the code base improves over time through these small changes. Keeping that in mind, any critique that happens as a result of this aim in mind needs to be taken professionally and not to heart.
It’s easy for us to get attached to our code, this is an obvious yet sometimes overlooked symptom of someone working by themselves for too long. Any critique of the code immediately gets mistaken for a direct offense to the person themselves. We’ve all been there, some could struggle out of lack of confidence and would need to try harder to not associate themselves with the possible improvements they need to make.
One particular excerpt stuck out to me which is:
“Never respond in anger to code review comments. That is a serious breach of professional etiquette that will live forever in the code review tool.”. No truer words have been said, that could be many people’s first impression of us in the future. In the case that this does happen, and we know we are at fault, apologizing off the record would firstly be a good idea. Also ensuring to remember that pull request comments are there out of respect for us as authors and the code helps in separating our emotions from code.
Fix the Code
If the reviewer’s comment is saying they are finding it difficult to understand the code, chances are that our code could be simpler/clearer and that future readers may ask the same question. Our first approach would be to attempt to clarify the code in question, if it’s not possible then we should give a detailed explanation of what is happening to give a full context for the reason for that approach. This helps the reviewer “get into your mind” and figure out a way it could be made better.
Think for Yourself
This sub section’s header leads me to a slightly different thought than what is written there already. I feel that this part is based on the confidence/personality of the change author in question. Going back to the Senior vs junior, easy going vs hard headed sub section in part 1, a very easy going change author needs to pay extra attention to the extent of their “easy-going-ness” (🤷♂️ I’m making words up at this point) in these situations. A light comment that could be proven incorrect could be taken as gospel by this sort of personality; resulting disregarding their own work even if it was correct.
I’m a recovering victim of this mentality, and it sucks. Although it can be regarded as a lot of work for some, what I did to help in overcome this is to anticipate where (not always what) the questions will land in my code; and see if I can answer them honestly. If I do find some areas, and cannot change or answer, I would include it in my “areas of concern” part of the pull request description.
Be Thankful 🙌
Another person has taken time out of their day to potentially help us become a better dev through these comments, so why not reciprocate? We could always end/start replies with a sense of thankfulness and appreciation. This habit is sometimes ignored, showing thankfulness and positive attitude will encourage more of it from that code reviewer in the future; giving us more chances to improve ourselves and the code base. Everybody wins!
With this third part in the series I have summarized, and added my own input, on how to conduct a code review as a change author. We looked at the actions we can to take in particular situations, before and during a code review, that will help a change author and the code code reviewer navigate it smoothly.
Please let me know if there's anything you agree/disagree with or pop a comment below to let me know what you think in general (and if there's any mistakes I made 🙏). I hope you enjoyed this series, I know I have and it opened my eyes a lot about myself and where I can improve. I hope it did the same for you too! 👋
Top comments (0)