Context

Code review is a very important task in the process of getting code changes ready to be merged. It consists of reviewing the changes proposed by a contributor and ensuring that it satisfies our contribution guidelines, from code style to organization and even correctness. We expect code reviewers to take it very seriously and spend the necessary time to throughly validate a pull request. As reviewing code can be time consuming, especially for long pull requests, we also expect contributors to be conscious of the work of code reviewers and help by:

• Writing a good description of the changes while pointing to critical aspects of the code changes to help the reviewer navigate through the pull request.
• Being receptive of comments. Your way is not necessarily the only way, so be open to different approaches or directions. Help the reviewer build an understanding of the changes if necessary as the reviewer initially might not fully understand it.

Validation via code reviews can help us to catch problems early. Be grateful that others are spending time with your changes and make everything in your hands to keep reviewers engaged.

This document has two parts, one for the contributor and one for the reviewer.

Starting a pull request and selecting reviewers

I have a set of changes and I want to contribute my code, what do I do?

The very first step is to make sure that the code changes follow the contribution guidelines. Once ready, submit a pull request, making sure that the pull request has at least one associated issue. That's all described in the contribution guidelines, so we are not going to say it again here.

The next step is to assign some reviewers. If you have some experience with the project, then you probably have an idea of who would be a good set of people to review your pull request. If you have little idea, say because you are a new contributor or not very familiar with the part of the code you are touching, then you can ask for help in the pull request itself. Some of the selected reviewers might also add other reviewers when they feel the pull request needs additional input. Otherwise, here are some points to keep in mind when selecting reviewers:

1. Be objective. Do not select your best buddy because he or she will resolve it fast for you.
2. Consider adding some expertise diversity. For more complex pull requests, consider adding people with different expertise for different points of view.
3. Don't add more people than necessary. Of course, one can be extra cautious by requesting a review from every single committer in the project, but that's rarely necessary and a waste of people's bandwidth.

Once the pull request starts getting comments, the contributor is expected to reply and address issues accordingly until all issues are clarified. A pull request is not ready until all reviewers have approved it.

Reviewing code

You're assigned to review a pull request, now what?

Start by reading the description of the pull request and the associated issue or even a PDP when applicable to get the context and what the contributor is trying to achieve. If the description is poor, then feel free to raise it with the contributor. It is important that the reviewer understands what the contributor is trying to achieve.

When reviewing the code, there are different aspects to pay attention to:

1. Soundness. Is the high level change sensible? Convince yourself that the change solves the problem that it is proposing to solve.
2. Side-effects. Do the code changes introduce undesirable side-effects? Perhaps the change will make an API change that breaks functionality or is not backward compatible, or perhaps it will create a performance bottleneck. Raise all such concerns.
3. Correctness. One level down, is the code correct? It might make sense to use a given data structure, but is it implemented correctly?
4. Concurrency. Is concurrency handled correctly in the code? Thread-safety is very important and we have guidelines for it. If you're a reviewer and not familiar with our concurrency guidelines, we suggest you go check it out (Threading guidelines).
5. Style, format and naming. Is the code structured properly? Naming and packaging are important for clarity and visibility. It is good practice to follow the same nomenclature throughout the project rather than inventing a different name each time. Learn what terms are being used in the case you are not sure. Don't make classes and methods public unnecessarily.
6. Tests. Is there enough testing and do the tests follow the general guidelines of the project? For example, if a test case is unnecessarily complex, broken or has a long timeout, then those should be raised as concerns.
7. Build. Does the build pass? Do not approve a pull request for which the build is not passing. There are various things checked in the builds: compilation, unit tests, and code coverage.

There are clearly many aspects to pay attention to if we are to perform a thorough code review. Sometimes, it is useful that reviewers coordinate so that each person can focus on specific aspects rather than try to consider them all.

As the reviewer finds concerns and questions to raise, they are raised in the pull request in the form of comments. Very important: do not be shy to say that you haven't understood a given part of the pull request. By doing it, the reviewer is missing an opportunity to understand that part of the code better and might be letting a real issue pass. Raising questions helps the developer to realize aspects that might not be entirely clear or that might be even broken. Staying quiet really does not help anyone.

The reviewer only approves the pull request when satisfied with the changes and the answers. Until then, the reviewer should have the review status as "Request changes". A common question is when to mark the review status as "Comments" rather than "Request changes". The use of "Comments" is confusing because it does not send a clear message to the contributor about how strongly the reviewer feels about the comments. It is better that the reviewer firmly request changes, and that the contributor happily accepts comments to help improve the pull request.