How to Write Good Code Reviews
FleetingBest Practices for Code Review | Learn Code Review
Review fewer than 400 lines of code at a time
The Standard of Code Review
he primary purpose of code review is to make sure that the overall code health of Google’s code base is improving over time. All of the tools and processes of code review are designed to this end.
reviewer makes it very difficult for any change to go in, then developers are disincentivized to make improvements in the future
reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect
no such thing as “perfect” code—there is only better code
Instead of seeking perfection, what a reviewer should seek is continuous improvement
Reviewers should always feel free to leave comments expressing that something could be better
be weighed on those principles, not simply by personal opinion
. If the author can demonstrate (either through data or based on solid engineering principles) that several approaches are equally valid, then the reviewer should accept the preference of the autho
If no other rule applies, then the reviewer may ask the author to be consistent with what is in the current codebase, as long as that doesn’t worsen the overall code health of the system.
When coming to consensus becomes especially difficult, it can help to have a face-to-face meeting or a video conference between the reviewer and the author, instead of just trying to resolve the conflict through code review comments
Don’t let a CL sit around because the author and the reviewer can’t come to an agreement.
What to look for in a code review
For changes like that, you can have the developer give you a demo of the functionality if it’s too inconvenient to patch in the CL and try it yourself.
“Too complex” usually means “can’t be understood quickly by code readers.” It can also mean “developers are likely to introduce bugs when they try to call or modify this code.”
over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system
solve the problem they know needs to be solved now, not the problem that the developer speculates might need to be solved in the future. The future problem should be solved once it arrives and you can see its actual shape and requirements in the physical universe
A good name is long enough to fully communicate what the item is or does, without being so long that it becomes hard to read
Usually comments are useful when they explain why some code exists, and should not be explaining what some code is doing
mostly comments are for information that the code itself can’t possibly contain, like the reasoning behind a decision