Book: Software Engineering at Google
Chapter: 9 - Code Review
Summary:
Code review has practical uses e.g. detecting bugs, but also more subtle, psychological and cultural effects. Code review should aim to prevent duplicated code, as it is not only wasted effort, but a liability in the long run. Very often, if someone is writing entirely new code, something is wrong, it may be that the author has forgotten to utilise utility code or existing library.
When performing review, the reviewer should check for the correctness and comprehension of the code:
Correctness of code
Check to ensure that the code does not introduce future bugs. Ensure that it has proper testing, proper design, functions correctly and efficiently.
Reviewer should not propose alternatives because of personal opinion, unless it improves comprehension (simpler solution) or functionality (more efficient solution).
Tools e.g. automated testing can often help with this process
Comprehension of code
Code review is the first test of whether a given change is understandable to a broader audience, this is important because the code will eventually be read many more times than it is written
Often useful to find a reviewer who has a different perspective from the author
Reviewer should be someone who might need to maintain that part of the code
Code consistency
The reviewer should check that the code needs conform to some standards of consistency so that it is readable, understandable and easy to maintain.
A rule of thumb is simpler code is often better.
Code correctness and comprehension are two main criteria for an LGTM!
After an LGTM is given, several levels of approval is needed before the changes can be merged:
Code owner needs to approve to add the code to their part of the codebase: Google’s codebase is a tree structure with hierarchical owners of particular directories. The owner can be a tech lead, expert who has been assigned the ownership privilege. Ownership does not mean ownership over the source code, but a sense of stewardship to act in the company’s best interest with a section of the codebase. It is a key mechanism which allows tens of thousands of engineers to operate efficiently on billions of lines of code in a single repository.
Approval for language style and best practices from readability reviewers. Readability approvals can only be granted by engineers who have gone through code readability training, e.g. Java code requires approval from an engineer who as “Java readability”
How important is code review besides ensuring correctness and comprehensibility?
Knowledge sharing
It turns out that through code review everyone learns something new. We often ask why a change is done in that particular way, this exchange of information between the author and the reviewer facilitates knowledge sharing. Both parties can learn new techniques and patterns from code review.
Psychological and cultural benefits
Code reviews reinforces to software engineers that code is not “theirs” but in fact part of a collective enterprise. This prevents engineers from gravitating towards their own personal style and approach to software design.
Code review also acts as validation and recognition for one’s work.
To streamline and expedite the review process, there are several guidelines we can follow:
Write small changes (when possible)
Large changes take a long time to review - small changes prevents engineers from wasting time waiting for reviews
It is easier to determine the source of bug down the road if the change is small
Writing good change descriptions! A change description should have the following
The type of change on the first line (as summary)
Details of what is being changes and why.
Enumerate related modifications (i.e. the historial record for this change)
Keep reviewers to a minimum
The most important LGTM is the first one
For subsequent reviewers the cost of additional reviewers quickly outweighs their value
Automate where possible
Automatic static analysis is important
Done during the presubmit process (before the change is sent to the reviewer for review)
Lastly, the chapter covers the types of code reviews and how we can handle such reviews in a manner that ensures the desired outcome of correctness and comprehensibility.
Greenfield code reviews
For large and mature codebases, this type of code review is uncommon, where entirely new code is added. However, this is the most important time to evaluate whether the code will stand the test of time.
We should question the underlying assumptions of the code - will it be easy to maintain as time and scale changes?
New code should undergo an extensive design review, and not leave it to the code review. A code review is not the time to debate design decisions!
A greenfield review ensures that API matches an agreed design, it is fully tested, with all API endpoints having unit tests, and the code should have proper owners, with sufficient documentation.
Behavioural changes, improvements and optimisations
Most changes fall under this category, including modifications to API endpoints, improvements to existing implementation, performance optimisation etc
As reviewers, we should ask ourselves: Is this change necessary? Does this change improve the codebase?
Augmentations should undergo Continuous Integration to ensure that modifications do not break underlying assumptions of existing tests
Optimisations should not affect any tests and might need to include performance benchmarks for the reviewers to consult
Bug fixes and rollbacks
Avoid the temptation to address other issues! Focus on solely fixing the bug and update associated tests to catch the error occurred in the first place.
Rollbacks reverts the previous change to a known state, but still require a code review. It is critical to ensure that a rollback does not cause further breakages due to dependency issues. This can often happen when dependencies are quickly drawn to newly submitted code, so we need to be mindful there.
Refactoring and large scale changes
These changes are often machine generated, but still requires review. The review process is same as any other code review, where the correctness, applicability etc are examined.
Book: Software Engineering at Google Chapter: 9 - Code Review
Summary:
Code review has practical uses e.g. detecting bugs, but also more subtle, psychological and cultural effects. Code review should aim to prevent duplicated code, as it is not only wasted effort, but a liability in the long run. Very often, if someone is writing entirely new code, something is wrong, it may be that the author has forgotten to utilise utility code or existing library.
When performing review, the reviewer should check for the correctness and comprehension of the code:
Code correctness and comprehension are two main criteria for an LGTM!
After an LGTM is given, several levels of approval is needed before the changes can be merged:
How important is code review besides ensuring correctness and comprehensibility?
To streamline and expedite the review process, there are several guidelines we can follow:
Lastly, the chapter covers the types of code reviews and how we can handle such reviews in a manner that ensures the desired outcome of correctness and comprehensibility.