This is a summary for the āWhat to look for in a code reviewā part of the code reviewer's aspect in Googles best practices for a code review, it's part of the main article Conducting a code review: Part 2 - The reviewer. We will be summarizing the subsections and grouping them in order of relatability. Itās a given that they are all linked in one way or another but some are just more so than others.
Design, functionality and complexity
As the design subsection states, āThe most important thing to cover in a review is the overall design of the CLā. Is there something that we need to solve in order to make this change at this moment in time, and how does this code fit in with what we are trying to solve? Does the change make sense? If we answer yes to these questions, then we can move on to thinking about the functionality aspect of this change.
The most obvious question is does this code build and work? Most of the time, we would get alerts in whatever devops tool we use to tell us that the build failed so this would let us know straight away. Is the functionality of the code consistent with what the change author intended? It makes sense to check the context of the ticket/task theyāve been assigned and try to see if they achieved what they set out to do in that piece of code. Google uses a UI change as an example, it can be hard to visualize the change in UI just through. In this particular case a demo by the change author is encouraged, second prize would include things like screenshots from different browsers to show what the change looks like (explaining steps taken to get to it).
When it comes to complexity, sometimes devs make things a little too complex to the point where the PR can sometimes be a pain to read into (we're all guilty at some point or another). Does this also mean that there is a chance that this could introduce bugs when we attempt to change or use this code? Possibly, in this case we would be encouraged to let the change author walk us through the intention behind the code (and a possible demo maybe). This would help both parties figure out the process and maybe make some suggestions on how to make this code simpler.
Tests
All types of tests that would apply to the case (unit, integration or E2E) are encouraged and should be asked about for the change. For unit tests, there are some tools that help with checking code coverage such as Sonarcloud. Code coverage can be used as one of the requirements in the code review process, letting this tool do all the work in figuring out what hasnāt been covered in test cases.
It is also useful to check if there is a chance that a test could provide false positives, this may arise from the test not asserting the correct parts of the code.
Naming, style and consistency
The naming of a particular thing should be relevant to the situation and precise enough to read and understand in a single second if possible. Every company has/should have their own style of naming or coding which should be adhered to. It lands everyone in the same style mindset when trying to understand code. When reviewing code, we should keep the style consistency in mind and compare it to what we see. In cases that a particular part of the code is not covered in the style guide, we would use more local aspects to dictate our consistency (such as what the current class has as a style).
As a code reviewer, there are times where we might like to ask the change author to change the style of something that may cause larger changes without our knowledge. We should refrain from major style changes, for both the change author and reviewer, and separate these in different pull requests: One for the functional change and the other for the style change.
Comments, documentation and context
Itās a good idea to also look for any comments in the code or in the general place of the code review to see if there is any extra context to why a piece of code was written in particular way. If there are, are they understandable and readable? Does it need to stay within the code (can it be taken out)?
Any large change that alters āhow users build, test, interact with, or release codeā should be documented in any documentation platform that the company uses. This would also include specific projectsā READMEs which usually live in the same repo. We shouldnāt hesitate to ask for documentation if we feel that this changes any of what was mentioned.
In some cases, we may look at code and not understand the context of it all. Looking at the code as a whole and not just the part that changed would help in identifying exactly what is happening. As mentioned previously, checking out the task assigned can also help us in seeing how the change author arrived at this solution.
One thing that was mentioned in the context section that could be deemed an experience dependent ability is checking if that piece of code degrades the code health of the system. Probably the more experienced one is, the more likely theyād be able to spot this. Being upfront and direct about what we don't understand ultimately improves the health of the code base, because devs want to make things that others can understand and use with ease.
Checking every line and the exception to checking every line
Itās generally expected for reviewers to read every human-generated line of code written within the code review, however there can be exceptions in the form of generally large changes and overly complex solutions which are difficult to get the context of or understand. Both of these scenarios encourage communication with the change author.
There are moments where there are other reviewers who have joined in, and we may not have all the time on our hands to look at the whole code change (if too large). In these cases, we can be upfront and direct, give the position of where we stopped and assess whether it looked good enough at that point with some comments if applicable.
Compliments to the chef š (Good things)
If we see something that can be made better and comment on it, we can do it for positive things that we see too. We shouldnāt hesitate to give positive reviews/comments whenever we see something dealt with in a way that was clever or is going in a great direction.
Some of us devs like to hear compliments to know that weāre going in the right direction, positive communication does wonders for the chemistry of the team. Although, we also have to gauge how people would react to compliments; some might not see it the way mentioned. In some cases, some devs see it as a useless distraction from what needs to be improved in the PR; read the room, I guess š¤·āāļø.
This summarized the āWhat to look for in a code reviewā section of Googleās best practices for code reviews. Itās always important to remind ourselves that every action is made with the intention to improve the code base's health over time, with every PR. It might be difficult to try and remember to do these all at once, but as long as we gradually integrate these things into our processes, we'd be going in the right direction.
Oldest comments (3)
Complimenting others also ensures we are all in the same page and the reviewers have understood your efforts and realizes that is the "best for today" implementation. š
Very true! The "best for today" part reopened my eyes to what a solution is in general, it constantly changes. You are 100% correct, it's a good solution for now š we never know what's around the corner!
š š