Efficient code review

Code review can be lengthy and stressful, especially if change is complex.

If the change in question is very important and may have severe consequences, it is important to weigh diverse input and to arrive at an agreement or a reasonable compromise. Addressing valid concerns can reduce the risks of introducing a change.

Sometimes code review turns into an arena with clashes of strong opinions, this is not always necessary.

Code review takes resources that could have been spent in development. What does it bring in return?

• Meaningfulness: reviewer can check that change makes sense from both problem and solution standpoints,
• Correctness: reviewer can check that code change is sufficiently tested and does not have obvious logical flaws, typos/misspells, obsolete documentation,
• Safety: reviewer can flag malicious, obscure or unreliable code,
• Suitability: reviewer can check if the change meets performance expectations, reasonably utilizes available resources, and does not conflict with already existing code.
• Consistency: reviewer can flag code that violates project conventions and/or common best practices,
• Knowledge: reviewer and contributor can learn from each other by asking questions about particular decisions or by suggesting alternatives.

All the checks above are usually the best effort to the best knowledge of a reviewer, because doing a thorough and comprehensive assessment may be prohibitively expensive.

Scope Of a Change

Smaller changes are easier to manage. Small atomically correct changes are typically less risky. Small changes are easier to review and comprehend. Small changes are not always feasible to deliver (especially in code with tight coupling between the components), but they are something to strive for.

The pull request (change set) should have a primary goal clearly articulated in description. If code changes are directly relevant to the primary goal, that often allows reaching a minimal acceptable size of changes.

It might be tempting to put more changes in, to follow boy scout rule. But this bears risks of unbounded growth which may delay the primary goal delivery or even make the whole change set too big for a confident review. If an improvement idea arises during code review and if it does not directly contribute to the primary goal, it is worth considering delivering such improvement as a separate change.

Suggestions And Change Requests

Apart from clarification questions, code review can end with approval, suggestions, and change requests.

Change requests are blocking pull requests from being merged.
A change request should indicate a reason why original changes are not ready to be merged.

Change requests should be used to address insufficient meaningfulness, correctness, safety, suitability, and consistency (core properties).

If those properties are not violated, suggestions are preferred.

Suggestions, as opposed to change requests, do not block pull requests from being merged. They invite pull request authors to consider alternative solutions or additional changes that may be beneficial from reviewer perspective.

Suggestions can convey opinions on naming, layout, design patterns and other things that can not be backed by existing team conventions. Suggestions help to bring different perspectives in a friendly way and are often accepted by pull request authors. However, pull request authors have a right to politely decline a suggestion if they think it does not bring enough value for the risks (growth of scope, unclear impact on core properties).

Declined suggestions can still be delivered in a separate pull request to avoid scope creep, and they can pass through a separate code review.

Both suggestions and change requests can vary in how helpful they are. Helpful messages may contain actual changes that can be applied to the code in question.

Attitude

Miscommunications and misunderstandings can happen often. They can happen even more often in a diverse team, where people have their own biases and cultural background.

Miscommunication is harmful, from being just counter-productive time spending to a more severe issue when feelings are hurt.

It is very important to have positive assumptions. If a person is having negative preliminary bias or negative assumptions, such bias can color messages with neutral original intent into offensive ones, and can even lead to painful escalations.

While we're all people with emotions, pull request is a technical ground, sticking to technical language and technical argumentation together with explicit articulation can help to reduce miscommunications. If you feel offended by a message, take time to calm down and re-read the context with an effort to discover positive/neutral intent. There is a high chance that this was the original intent conveyed by the author.

Code review is a collaboration tool, we're doing it to be more successful as a team. The best outcome of a review is a quick and confident approval without any suggestions, such cases usually indicate a high level of harmony in a team.

Code review is not the best place to exercise one's own importance by nitpicking or asking for cosmetic/out-of-scope changes. If the suggestion feels important, but it meets justified resistance from a pull request owner and/or other reviewers, it might be a good idea to submit such a suggestion as a separate pull request.

Try to avoid making suggestions/change requests based solely on opinion. Opinions can vary from one person to another, and they can easily be a point of locked disagreement. The opinion of one person is not necessarily more significant than the opinion of another person, so there is a fundamental issue that may need the involvement of an authorized arbiter.

Technical argumentation with pros and cons (as opposed to just opinion) can lead to a balanced compromise or consensus.

Rules And Principles

• All parties are egoless and committed to reach ideal handling
• All parties take review process as an opportunity to learn
• All parties are trying to communicate in a timely manner to avoid blocking each other
• Review process does not take longer time than necessary
• Contributor articulates the problem to be solved, comprehensible by reviewer
• If the problem is a bug, contributor creates a test to reproduce the bug with CI before pushing a fix
• Contributor makes changes that are directly relevant to the problem described in PR
• Contributor avoids unnecessary changes
• Contributor formats changes in a way that is consistent with existing code
• Contributor applies boy scout rule when that does not violate above points
• Contributor creates additional tests to cover new behavior
• Contributor updates the tests to match changed behavior
• Contributor makes sure the problem is solved (best effort)
• Contributor is ready to provide justification for every change that was made
• Reviewer comprehends PR description and the problem
• If reviewer has a doubt or lack of understanding, they request a clarification
• If clarification is requested, contributor reasonably elaborates the topic
• Reviewer checks that problem of PR is worth solving
• Reviewer checks changes in tests to confirm that the goal of PR is achieved
• Reviewer checks changes in tests to confirm there are no unnecessary changes in behavior
• Reviewer checks that changes do not introduce a security/performance issue (best effort)
• Reviewer checks that changes do not have logical conflict with existing code (best effort)
• Reviewer makes helpful change requests if necessary
• Reviewer is ready to provide justification for change request
• Helpful change request is suggestive, precise, and relevant to the problem of PR
• Reviewer avoids unhelpful change requests
• Unhelpful change request is broad, opinionated, or not relevant to the problem of PR
• Contributor can challenge any change request
• If justification of change request is shown to be weak, change request can be discarded
• If helpfulness of a change request is questionable, reviewer can elaborate it to clearly helpful, or it can be discarded
• Reviewer can challenge any change of PR
• If justification of change is shown to be weak, contributor has to revert the change
• If there are helpful change requests that could not be discarded, contributor has to address them with PR changes

See Also

Code Review Developer Guide.