Efficient code review

This content originally appeared on DEV Community 👩‍💻👨‍💻 and was authored by Viacheslav Poturaev

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 usually 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.

Pull request (change set) should have a primary goal clearly articulated in description. If code changes are directly relevant to the primary goal, that usually allows to reach 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.

Attitude

Miscommunications and misunderstanding 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. Opinion of one person is not necessarily more important 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 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 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 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

Scope Of a Change

Suggestions And Change Requests

Attitude

Rules And Principles

See Also

Related Posts