Code Reviews are a software development process intended to be used principally as a quality assurance step. It allows engineers to request feedback from other engineers about the code that they wrote.
There is no single reason why Code Reviews are important. Code Reviews let engineers look at other engineers’ code from a different perspective, with a different mindset, and with a different level of seniority and knowledge.
The first reason Code Reviews are important is error prevention. Like all human things and human creations, the code is also affected by human flaws. Code Reviews are principally intended to prevent mistakes from getting through and reaching production. There are many reasons humans fail at doing things, especially at writing code: disattention, unhandled corner cases, missing knowledge, and so on. These reasons might not be detected by automated tests and QA processes because they are tricky.
Another reason Code Reviews are important is knowledge sharing, coaching, and collaboration. Especially in dynamic contexts and small teams, it’s not always possible to share in a structured way information about domains, products, processes, standards, and so on. Information in those contexts is mostly shared verbally. So, most of the time, Code Reviews are also used as a tool to share knowledge collaboratively. It’s a two-way tool to share information, both from the Code Reviewer's and Author’s perspective. Code Reviewers can get guidance and tips by looking at the code written by the Author. Conversely, Code Reviewers can share their point of view and feedback to give tips and guidance to the Author(s). An impressive tool that, if it is used effectively, can give an impressive boost to productivity.
On top of knowledge sharing, Code Reviews allow for better ownership distribution. Everyone who is involved in the review process becomes the owner of the code you work on. This produces a much healthier environment where everyone is responsible for the code and its responsibilities. In this way, there is no chance an individual engineer can be blamed for flaws that may reach production.
Lastly, Code Reviews benefit code health. Software engineers are lazy and there might be the chance that code is written only to get things done. Without a process that forces engineers to control the code quality of other engineers, the probability that the code degrades over time increases. Degraded code is less maintainable, which means a slower development pace, an extended timeframe during feature development, more defects, more bugs, and higher costs. So, degraded code directly weighs on the company costs.
The following are the key points that you, as a Code Reviewer, should keep in mind when reviewing someone else’s code.
It’s the most important thing to control during a Code Review. Here, are the main aspects to check regarding the architecture of the code. For example, you might ask yourself:
Here, as a reviewer, you might ask yourself if the current Pull Request you are reviewing is doing what it is supposed to do. For example, you might ask yourself whether the introduced changes reflect the specifications defined in the related Asana card, in the related Issue, or, possibly, in the Pull Request body.
In case the Pull Request introduces visual artifacts (e.g., User Interfaces, images) or consumable APIs, it’s always a good practice to check them to spot problems before QA. Here, the key point is manual testing and reproducing these changes. Therefore, the Pull Request should be executable in isolation. If this is not possible, you might ask the Author for a demo.
Overall, the key point here is to take the POV of the final user to extract the best observations you can find.
Adding unneeded complexity is a real problem because it threatens the sanity of the codebase by adding tech debt and complex code that must be maintained in the future by other developers. A codebase that is too complex is also susceptible to bugs that affect product quality and user confidence. Therefore, Code Reviewers must be on the lookout for over-engineering and over-complexity.
Tests should be added in the Pull Request and the code reviewer must assure this. Therefore, if there are no tests, don’t be afraid to request them. Adding tests means increasing the confidence in the output that we expect to be produced by our artifacts.
However, there are some cases in which this could not be possible. All these cases are generally defined as emergencies (e.g., hard deadlines, incidents). When they happen, it’s allowed to skip tests by adding todos (e.g., in Jest by using it.todo or test.todo or any other testing tool that permits that). Todos allow us to track missing tests that could be tackled in a future iteration (e.g., during a Cooldown phase). Even these cases should be rare if the PR author followed TDD.
A human must ensure a test is valid. We don’t write tests that test tests. Therefore, it’s also up to the reviewer to check whether the tests are valid or not.
Last but not least, tests must be simple and predictable. Simple tests allow for better maintainability. Predictable tests give us more confidence. Adding complexity and unpredictability in tests is a problem because it threatens future iterations and makes them susceptible to bugs. Therefore, as a Code Reviewer, always keep an eye on these peculiarities.
A good name is long enough to fully communicate what the item is or does, without being so long that it becomes hard to read. As a Code Reviewer, you can give your input as far as the naming being used and, if needed, give alternatives if you think a name can be enhanced.
Be vigilant regarding naming conventions. Usually, naming conventions (e.g., Hungarian notation, etc) should be avoided unless a codebase sets expectations in that direction by using a static checker (e.g., eslint).
The quality and sanity of a codebase can be also recognized by the naming that is being used.
Code comments should explain why the code exists instead of explaining what the code is doing. An exception to this last statement (i.e., they should not explain what the code is doing) could be the algorithm. They could end up being to be read by a developer. So, in case complexity can’t be avoided in other ways in algorithms, comments that are intended to explain what the code is doing are permitted.
Generally, a good practice is keeping an eye on components’ documentation. Documenting a class or a function does not count as a comment as long as the comment explains how to use the commented component.
The style used in a Pull Request must follow the style guide. The reviewer who wants to point out something about style can comment using Nit: indicating that the comment contains a nitpick.
In some cases, the style guide makes recommendations rather than declaring requirements. In these cases, it’s a judgment call whether the new code should be consistent with the recommendations or the surrounding code.
Always look at the documentation if it is missing. Don’t be afraid to request it. A documented codebase is readable, usable, and is not bound to the owners by reducing the bus factor. In other words, just ask yourself: what would happen in case a particular system is bound to a single owner and this latter dies hit by a bus?
The commit message should explain what the introduced changes are in a few words. You must pay attention to the semantics by asking yourself:
The imperative mood should be used to explain that the commit is bringing the codebase from state A to state B. For example, Add component Foo to handle async communication is an example of a good commit, Fixed CR issues are not because it does not tell the why nor do they use an imperative mood.
Sometimes, you might find yourself looking at long Pull Requests. Although the effort that the author(s) put into writing code is understandable and valuable, having long Pull Requests to be code reviewed is problematic. Having many changes at the same time to check within a single Pull Request makes your life harder as a Code Reviewer, giving you a hard time figuring out eventual problems. Most of the time, you’ll find yourself approving without even reviewing the Pull Request in detail.
Moreover, having small Pull Requests to review means velocity. Facing a small number of changes lets you iterate more quickly with the Pull Request’s author(s) in case problems arise.
Therefore, as a Code Reviewer, it’s up to you to ask the author(s) to split the Pull Request into many Pull Requests.
If you see something nice in the Pull Request, tell it to the Author(s), especially when they addressed one of your comments brilliantly. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices as well. It’s sometimes even more valuable, in terms of mentoring, to tell a developer what they did right than to tell them what they did wrong.
Moreover, evidencing the right thing in a Code Review has a positive impact on morale. Therefore, don’t waste your chances appreciating other developers’ work in case there is an opportunity.
Be kind.
Kindness. This is one of the most important things to consider and to use. Behind a Pull Request, there is a human with a different context, different experiences, different seniority, and a different background. Kindness will help you to reduce conflicts, churn, and anxiety and make this process smoother.
No one wants to receive a comment like the following one
You must refactor this function because is totally unreadable. There’s no way we are gonna maintain what you wrote.
It contains a personal attack (i.e., the Code Reviewer is directly targeting the Author(s) by saying that their code is unreadable) and it is totally rude. You might rewrite that comment like
I’m noticing that this function is quite long and contains many conditions.
In the long term, this could be problematic for maintaining it.
For example, the execution might need to be disambiguated due to .
Adding this disambiguation could make this function harder to be read.
Do you agree with that?
What do you think if we do this instead?
Emoji is another great weapon to explain in a glance the mood of a comment. Consider using them. 🤓
Giving context and explaining why you’re writing a comment is another thing to consider. It will help you reduce the time that you’ll spend on iterating on the same problem. Moreover, it will help you in sharing knowledge. Newcomers might not be aware of quirks, edge cases, production specifications, workaround, and so on (there are many things hidden in a Product sometimes). An example could be the following one
In JavaScript, by using Promise.all, we wait for the entire set of Promises to be executed and resolved. However, at the same time, Promise.all will reject in the case one of the awaited Promises will reject.
Since we should not fail due to this product specification, we might consider using Promise.allSettled to get instead a resolved Promise that will contain the results of each awaited Promise. With these results, we might do .
Is the problem clear? Do we give it a try?
In case you have been selected to do a review of a Pull Request that contains multiple reviewers, exceptions are permitted. The key point here is to set expectations on the output you are supposed to give.
For example, you might review only some parts of the Pull Request (e.g., a file or from a high architectural level). These cases are good opportunities for using LGTM with some comments, to indicate the things you reviewed and set expectations.
You might also wait for the other reviewers to give your approval. In these cases, set the expectations by commenting on the Pull Request under review saying that you’re waiting for something (e.g., LGTM, but I’m going to approve this Pull Request after John Doe gives their approval).
Sometimes as a Code Reviewer, you’ll find yourself giving feedback on small things, problems that do not impact the final artifacts. Problems that are minutiae. Pointing those problems out by blocking the Pull Requests causes churn, hits morale by putting anxiety on the Author(s) and delays Pull Requests getting through to production. This is undesirable for velocity and cost reasons.
Since these are not problematic at all, a more healthy way is to point them out by adding a comment starting with Nit:, like the following example describes:
Nit: Adding a space over this line enhances readability of this function.
Or, in general, by explaining in other ways that the comment you are writing is not blocking the Pull Request.
In this way, you can indicate to the Author(s) that this comment is not the real problem in the Pull Request making the process smoother. In the end, most of the time, the Author(s) will fix these comments anyhow but without causing the problems mentioned before.
You might fall as a reviewer in a state of conflict with the author(s) of the Pull Request that is being reviewed. Don’t be afraid, this is a common thing, especially on tough Code Reviews.
Therefore, three steps for resolving conflicts during Code Reviews to avoid Pull Requests sitting around are: