Google's Engineering Practices – Code Review Standards

We dig into Google's engineering practices documentation as we learn how to code review while Michael, er, Fives is done with proper nouns, Allen can't get his pull request approved, and Joe prefers to take the average of his code reviews. In case you're reading this via your podcast player, this episode's full show notes can be found at https://www.codingblocks.net/episode132. Be sure to check it out and join the conversation. Sponsors University of California, Irvine Division of Continuing Education – One of the top 50 nationally ranked universities, UCI offers over 80 certificates and specialized programs designed for working professionals. Registration is NOW OPEN! Sign up and reserve your seat today! Datadog.com/codingblocks – Sign up today for a free 14 day trial and get a free Datadog t-shirt after your first dashboard. Survey Says Do you *always* include (new or updated) unit tests with your pull requests? Take the survey at: https://www.codingblocks.net/episode132. News Thank you to everyone that left us a review: iTunes: Jbarger, Podcast Devourer, Duracce Stitcher: Daemyon C How to Code Review Code Review Developer Guide Q: What is a code review? A: When someone other than the author of the code examines that code. Q: But why code review? A: To ensure high quality standards for code as well as helping ensure more maintainable code. What should code reviewers look for? Design: Is the code well-designed and appropriate for your system? Functionality: Does the code behave as the author likely intended? Is the way the code behaves good for its users? Complexity: Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it in the future? Tests: Does the code have correct and well-designed automated tests? Naming: Did the developer choose clear names for variables, classes, methods, etc.? Comments: Are the comments clear and useful? Style: Does the code follow our style guides? Documentation: Did the developer also update relevant documentation? Picking the Best Reviewers Get the best reviewer you can, someone who can review your code within the appropriate time frame. The best reviewer is the one who can give you the most thorough review. This might or might not be people in the OWNERS file. Different people might need to review different portions of your changes for the same pull request. If the "best" person isn't available, they should still be CC'd on the change list. In Person Reviews If you pair-programmed with someone who was the right person for a code review, then the code is considered reviewed. You can also do code reviews where the reviewer asks questions and the coder only speaks when responding to the questions. How to do a Code Review The Standard of a Code Review The purpose of the code review is to make sure code quality is improving over time. There are trade-offs: Developers need to actually be able to complete some tasks. If reviewers are a pain to work with, for example they are overly critical, then folks will be less incentivized to make good improvements or ask for good reviews in the future. It is still the duty of the reviewer to make sure the code is good quality. You don't want the health of the product or code base to degrade over time. The reviewer has ownership and responsibility over the code they're reviewing. Reviewers should favor approving the changes when the code health is improved even if the changes aren't perfect. There's no such thing as perfect code, just better code. Reviewers can actually reject a set of changes even if it's quality code if they feel it doesn't belong in "their" system. Reviewers should not seek perfection but they should seek constant improvement. This doesn't mean that reviewers must stay silent. They can point out things in a comment using a prefix such as "Nit", indicating something that could be better but doesn't block the overall change request. Code that worsens the overall quality or health of a system should not be admitted unless it's under extreme/emergency circumstances. What constitutes an emergency? A small change that: Allows a major launch to continue, Fixes a significant production bug impacting users, Addresses a legal issue, or Patches a security hole. What does not constitute an emergency? You want the change in sooner rather than later. You've worked hard on the feature for a long time. The reviewers are away or in another timezone. Because it's Friday and you want the code merged in before the weekend. A manager says that it has to be merged in today because of a soft deadline. Rolling back causes test failures or breaks the build. Mentoring Code reviews can absolutely be used as a tool for mentoring, for example teaching design patterns, explaining algorithms, etc., but if it's not something that needs to be changed for the PR to be completed, note it as a "Nit" or "Note". Principles Technical facts and data overrule opinions and/or preferences. The style guide is the authority. If it's not in the style guide, it should be based on previous coding style already in the code, otherwise it's personal preference. The reviewer may request the code follow existing patterns in the code base if there isn't a style guide. Resolving Conflicts If there are conflicts between the coder and reviewer, they should first attempt to come to a consensus based on the information discussed here as well as what's in the CL Author's Guide or the Reviewer Guide. If the conflict remains, it's probably worth having a face to face to discuss the issues and then make sure notes are taken to put on the code review for future reference and readers. If the conflict still remains, then it's time to escalate to a team discussion, potentially having a team leader weigh in on the decision. NEVER let a change sit around just because the reviewer and coder can't come to an agreement. Resources We Like Google Engineering Practices Documentation (GitHub) Code Review Developer Guide (GitHub) How to do a code review (GitHub) The Standard of Code Review (GitHub) Emergencies (GitHub) The CL author's guide to getting through code review (GitHub) Technical Writing Courses (developers.google.com) Ruffles Potato Chips, Cheddar and Sour Cream (Amazon) Flawless Execution Tip of the Week William Lin's competitive programming channel (YouTube) Register for the free Microsoft Build digital event, May 19-20. (register.build.microsoft.com) Apple to host virtual Worldwide Developers Conference beginning June 22 (Apple) Checkstyle helps Java developers adhere to a coding standard. (checkstyle.sourceforge.io) CheckStyle-IDEA – An IDEA plugin that uses Checkstyle but isn't officially part of it. (plugins.jetbrains.com) Black – The uncompromising code formatter for Python. (pypi.org)