CODE_REVIEW.md
Code reviews are both a means of collaboration and of quality control. A code review is a standardized way for a developer to show their code to another developer and receive feedback. It also allows for the independent verification of the quality and the correctness of new code.
You have to make sure you understand the code. First, because it is crucial to have clear and understandable code for its maintainability and extensibility. If you cannot understand the new code, other readers in the future will not understand it either. Think about a new contributor reading this code in 3 years from now and help them not trip over the same lines you did. If something was unclear or confusing to you, make sure it doesn’t stay like that. Ask the author to solve it, and optionally suggest how: by refactoring the code, by adding a comment, etc. You also have to make sure you understand the code so that you can reason about it, about its correctness, necessity, quality, etc.
A couple of the main questions to ask when reviewing code:
It is the responsibility of the code's author to make this very clear. A simple way for the code's author to do that is to link an issue that provides all the necessary information. In some cases, the issue alone might not provide the best context. Some examples for such cases are if the issue is not written clearly, lacks context or contains a very long discussion. In those cases, it is the responsibility of the code's author to provide clear information about the purpose of the change.
What is the solution chosen for the issue; how the new code works and what it does. It is the responsibility of the author to make sure this is clear to the reviewer, even without reading code. In some cases this information will be part of the issue. In other cases, it should be provided by the author.
It should be clear enough to see that the new code is actually doing what it is supposed to be doing. Tests are a good way to demonstrate that code is doing what it is supposed to do. Even large and complex processes should be broken down into small, simple components. This allows readers to understand the code and reason about it. It also makes it easy to test well. If reading the code and its test does not make it obvious that the code does what it is supposed to do, ask for changes that will make it clear (e.g., unit tests, refactoring).
A note about AI: information and explanations about the new code can be generated by AI. This does not mean the author does not need to include them. Unlike the reviewer, the author can immediately know if AI-generated explanations of their code are correct or not. Therefore, the author is responsible for including the necessary correct information in their PR (whether AI-generated or not).
The author is responsible for the correctness of any AI-generated content in their code and their PR.
The reviewer can, of course, use AI however they like during their review, and that does not take away from the author's responsibility for a comprehensive and comprehensible PR.
Both the author and the reviewer are encouraged to use LLMs as a first superficial review, that can easily find some small issues.
The reviewer must still look at the actual code, and not only at AI-generated summaries etc.
The reviewer should require new code to have good style and follow best practices. The repository's current standards should be the minimal bar for new code. Well-known good coding style and practices should be the goal, but should not be strictly required in cases where the current standards of the existing code are too far behind. In those cases, we should just require the new code's style and patterns to be better than the existing code's.
When requiring a style change, if possible, point to the relevant style guide and to where in the repository, it is documented that this style guide is to be followed. If there is no entry in the team's style guide, and no pointer to an external style guide on the matter, it is the reviewer's responsibility to add that documentation and seek the team's approval. A consensus in the team should be reached through a discussion (e.g., on Slack).
Style matters, but it should never be what is blocking a review, if anyone (users, other team members, a release) is waiting for the code to be merged. This means it is ok to require style changes, but a merge should not be blocked solely by a discussion on style. In such cases, the code can remain as written by the author. The reviewer can open an issue and assign it to the author. The issue's task is to await the team decision and once it is reached, update the code accordingly. That issue should be started immediately, as it is part of the same existing task and is only supposed to unblock a merge, not to delay the style improvement indefinitely.
The reviewer is responsible to get the team's decision and document it in the repository. The original author is responsible for applying that decision to their (possibly merged) code.
The reviewer should actively try to help the author write good code. This means keeping an eye out for things that can be improved, and sharing ideas and suggestions. Some things are a matter of taste. If they are not clearly settled by any style guide followed by the team, it is ok to make suggestions that are not dictated by any guidelines, but make sure it is clear which comments require resolving, and which comments are actually just suggestions that the author is free to ignore if they disagree.
If you see something beautiful, smart, helpful, nice or well-crafted, feel free to comment just to acknowledge that.
If an alternative approach is equally valid to the author's approach, the reviewer should not strictly require switching to the alternative approach. If the author and a reviewer cannot agree on something, they can go to the team as a whole or to a leader (manager, architect, etc.) to resolve their conflict.
It is ok to post comments but already approve the PR at the same time, for the sake of speed. This is appropriate, for example, when the comments are all optional or when the comments are straightforward and trivial changes that will not require another review.
Code contributions from external contributors are highly appreciated and welcomed. Nevertheless, they require more scrutiny and caution. Unlike with internal contributors, code written by external contributors must not be approved on the basis of trust (like when approving with unresolved comments). In that case the reviewer is responsible for verifying the correctness of the code independently, and also for performing active quality control, unlike when reviewing internally contributed code. External contributors are any contributors who are not currently under a contract with the company.
The reviewer is not responsible for performing active QA of the new code. If for any reason, the author would like to ask the reviewer to run their new code, they have to ask for it specifically and provide exact step-by-step instructions. In most cases, this should not be necessary, as the code should be verified to a reasonable degree by tests. However, in some cases it might make sense to get the reviewer to run the new code, like when making UI changes that might show up differently on different systems and could show up wrong in unpredictable ways that are difficult to test.