Book: SE@Google Ch: 14 - Code Review

[nknguyenhc]

Book: Software Engineering at Google
Chapter: 14 - Code Review

Summary:

What is code review

Code review is the process of reviewing someone else's code before introducing it into the code base.

Standards of code review

Code review must meet 3 criteria before being merged into the code base:

• Correctness, whether it performs the fix/feature as expected.
• Appropriateness, whether it is suitable for the code base/directory.
• Readability, whether others can understand and maintain the code.

Usually, for smaller pull requests, all 3 can be verified by one reviewer. However, for bigger pull requests, this can be split between a few people.

For NUS-OSS, for bigger pull requests, it is possible to split the review into such aspects. For example, juniors can test the correctness of the pull requests, while seniors can review the appropriateness and readability of the changes. Moreover, it is recommended that different reviewers focus on different aspects of the change.

Benefits

• Code review ensures correctness, comprehensibility and consistency of changes. This is for sure the primary reason that code reviews exist in the first place.
• Code review promotes team ownership. When writing code only for oneself, he tends to write code in his own style. However, writing code for a collective code base with a pull request workflow in place forces one to treat his code as team-owned, and conform to the code base's standards.
• Code review also enables knowledge sharing. Upon requesting changes, reviewers are sharing his knowledge about the issue with the author. The discussions in pull requests also serve as records for future developers to reference.

Best practices

• Be polite and professional. Sometimes requesting others to change their code can make it feel very personal. Keeping a polite and professional tone minimises such occurrences. Moreover, reviewer should only suggest alternatives if it has clear advantages over original solution, not out of personal opinion.
• Write small changes, focused on one issue. This is so that changes can easily be traced, and rollbacks can be performed.
• Write good change description. Be more specific that just literally saying "Bug fix".
• Keep reviewers to a minimum. Generally, one should be sufficient.
• Automate where possible. This usually applies to tests.

Generally, these best practices are already followed by our NUS-OSS projects. One thing that all of us should keep in mind is the first point, where we should only request changes when necessary and accept author's solution if it has no clear disadvantages, and keep the conversations professional.