When I embarked on my software engineering journey I was not really sure what the code review was. The term seemed to be quite clear and self-explanatory, but it took me years to realize how important this process was and how it should be approached. It also varies throughout the different companies, where each of them implements its own process.
While reviewing other people’s code, and getting reviews from my colleagues I sometimes feel the lack of standardization of the process. In some cases, it would be much easier to follow an established common procedure that would lead the reviewer through the process more quickly and effectively than digesting the code in a new way every single time.
The purpose of code review
The software development process is not just about designing the product and coding it. No matter how good the design is, it cannot work without ensuring the proper quality at each link of the development chain. Besides the automated quality assurance that’s usually embedded into the CI/CD pipeline (read more), a human factor – the code review – is inevitable.
Some can ask .. why?
Because the code is written mostly for other programmers, not computers.
The computers don’t care about software architecture, variable naming conventions, modules, build systems, etc. Executing instructions – that is their essential task. They don’t even need a programming language for that. Right out of the compiler, our amazingly structured and well-organized program is just a binary consisting of CPU instructions accompanied by some memory addresses and arbitrary data.
We – programmers – need programming languages to effectively build our programs and communicate them to others from our species. To ensure the correctness, maintainability, and readability of our code, we need to show it to somebody else.
The reviewer’s responsibility for the code’s quality
People make mistakes. Since software developers are mostly people they may make mistakes as well. Even though some of them won’t admit it. No matter how good a software engineer is, an error can silently slip into the code. Another pair of eyes usually look at a code from a different angle. As we can overlook some details while sitting in the developer’s chair, the external assessment is very important.
The reviewer’s task is to notice mistakes and prevent them from being committed to the codebase. A suggestion of the way of reimplementing some piece of logic is usually also appreciated. It is essential to remember that any issue that slips through a code review and ends up in production, can bite not just the programmers, who created the erroneous piece of logic, but also other team members including the code reviewer.
The code review essentials
The following is the list of hints that have been evaluated from a number of pull requests I created and reviewed. It is a compilation of my experiences and thoughts I wish to save. They help me give better feedback which benefits in an increased quality of the code and decreased frustration regarding the code’s correctness afterward.
0. Learn more about the proposed changes.
Start with finding out what problem the patch is supposed to solve. The easiest way is to ask the author for an explanation or find the description in the associated ticket. Maybe an explanation has been already put in the Pull Request description field?
If you know the problem it will speed up and simplify the review, because it will become clear what to focus on in the first place.
1. Focus on the essentials.
Have you learned what the code is supposed to do? Great! Now is the time to prioritize the most crucial parts of the patch.
What are the most important bits and how to find them?
It differs depending on the project, the task, and the patch, however, here are my generic rules that can help you decide.
- If the patch is about adding or changing the logic – focus on the parts containing the relevant parts ignoring the config files, documentation, comments, and files moved.
- As you get to the most relevant parts, assess the quality of introduced changes by either imagining how you’d implement that or how it differs from the previous version.
- If you are unsure what has the highest priority, you can always try to ask the author for help with identifying it.
2. There is only what you see.
It took me a long time to realize and understand this phrase in the context of code review. IMHO This is also one of the hardest aspects of thinking in general. We tend to focus just on the data we are given, neglecting everything else. In the context of the code review, we usually think about what a given patch does, omitting the aspects of what it does not, or what it should do. Sounds too convoluted?
The best examples of omitting this rule are overlooked edge cases that are neither covered by automatic tests nor by the reviewer.
Handling such edge cases requires some code. That code would be noticed by the reviewer if existed. But.. the code does not exist, so nobody can check it unless the reviewer predicts such conditions which aren’t handled in the patch.
While reviewing the code, try to predict unforeseen conditions that may happen to the new/changed code. Those can be, for example, passing wrong arguments to the function, using the class/modules the other way the author assumes, using the naming convention that would collide with other modules, or even introducing legacy/harmful dependencies.
3. Ask questions.
Approaching the softer side of code review, we face the limitations of our mental characteristics. If you find an issue in the code, don’t be afraid to point it out. Remember that everyone makes mistakes, and even if the author is more experienced, you are still eligible to bring valuable feedback.
Hint: In case you are unsure about your comment, you can paraphrase it as a question. If the commented fragment indeed needs a change, the author will address your doubt and correct the code. Otherwise, you’ll learn something, which is also highly appreciated.
4. Mind your tongue.
Still walking on the soft side of the developer’s skillset spectrum, we are inevitably approaching the most important aspect of communication – language. It is essential how we communicate the correction requests and suggestions related to the reviewed patch. Remember that the piece of code you’re checking is the result of another person’s work and the way we express our doubts and suggestions may cause the author to feel various emotions. Try to avoid imperative sentences and ask for changes instead. Also, avoid being sarcastic or passive-aggressive. These behaviors do not belong to a professional environment. Let’s look at Example 1 which shows theoretical comment and its better version.
Example 0.
Consider the following comment:
This doesn't workIt would be much better if it pointed out what exactly the issue was and why it didn’t work.
It can rephrased to look like this:
In this case, X won't do the thing Y because Z. Please correct X.In addition, the author needs clear suggestions for changes to save her/his time implementing them. For this reason, the comments should be concise and accurate. Looking again at Example 1., the corrected comment suggests where the issue is and what to focus on to fix it.
Hint: If you and the author don’t belong to the same cultural circle, it may be a good idea to learn more about the author’s culture. Cultures differ, and some behaviors though as kind in one culture, can be seen as rude in the other.
5. Don’t like it? Suggest an alternative and justify.
Looking for alternative, and better-suited solutions during review is usually also welcomed. When you find out that the patch works as expected, but it could be better in some way, the review process is a good time to speak it out loud.
Hint: You can add your version of the code in the comment and write a few sentences of justification. For example, Github enables it by emphasizing an alternative version of code by enclosing it with a suggestion block and allowing the author to commit it right away.
Example 1.
```suggestion
// The suggested changes
```
What about changing the code `foo` to `bar` to be more aligned with the spec?6. Code is easy to add and hard to remove.
Do you remember having difficulties understanding how the code written a couple of months ago actually works? Did you ever have an opportunity to remove dead code from the project and make sure that nothing breaks afterward?
If you answered the above questions, it is highly probable that the answers do not bring any happy experience.
As a reviewer, you can (and IMHO should) prevent the bad or useless code from being pushed into the codebase.
7. Take it as a lesson.
Every patch can be a good opportunity for a reviewer to learn. If the presented solution is well-written a reviewer can learn from it, and understand some new tricks. It can also be a good challenge to find issues within good-quality code.
On the other hand – looking into a new code being added to the project helps us stay up-to-date with the codebase, which may save time while developing our future patches.
Summary
Congratulations to those, who reached here!
The code review process is a great tool that helps to eliminate bugs and prevent logic issues from being pushed into production. It also helps developers to learn about the most recent changes in the project and skill up problem-solving expertise by searching for bugs in other people’s code. All this combined creates a good exercise for both experienced and beginner programmers.
The presented list of code review hints aims to help developers become better reviewers and be more conscious while compiling their feedback for others. Remember, that people differ and a new way of approaching others sometimes needs to be created. A wide spectrum of problems have been covered by the list and I hope that it will be helpful for developers searching for inspiration or motivation while stuck in the middle of a bunch of code reviews waiting for their attention. I believe there are people who, like me, try to understand the problem and its solution avoiding mindless acceptance of all proposed patches.
Software engineer specializing in embedded development.
An enthusiastic overthinker.