diff --git a/LICENSE b/LICENSE deleted file mode 100644 index 1a16e05..0000000 --- a/LICENSE +++ /dev/null @@ -1,319 +0,0 @@ -Creative Commons Legal Code - -Attribution 3.0 Unported - - CREATIVE COMMONS CORPORATION IS NOT A LAW FIRM AND DOES NOT PROVIDE - LEGAL SERVICES. DISTRIBUTION OF THIS LICENSE DOES NOT CREATE AN - ATTORNEY-CLIENT RELATIONSHIP. CREATIVE COMMONS PROVIDES THIS - INFORMATION ON AN "AS-IS" BASIS. CREATIVE COMMONS MAKES NO WARRANTIES - REGARDING THE INFORMATION PROVIDED, AND DISCLAIMS LIABILITY FOR - DAMAGES RESULTING FROM ITS USE. - -License - -THE WORK (AS DEFINED BELOW) IS PROVIDED UNDER THE TERMS OF THIS CREATIVE -COMMONS PUBLIC LICENSE ("CCPL" OR "LICENSE"). THE WORK IS PROTECTED BY -COPYRIGHT AND/OR OTHER APPLICABLE LAW. ANY USE OF THE WORK OTHER THAN AS -AUTHORIZED UNDER THIS LICENSE OR COPYRIGHT LAW IS PROHIBITED. - -BY EXERCISING ANY RIGHTS TO THE WORK PROVIDED HERE, YOU ACCEPT AND AGREE -TO BE BOUND BY THE TERMS OF THIS LICENSE. TO THE EXTENT THIS LICENSE MAY -BE CONSIDERED TO BE A CONTRACT, THE LICENSOR GRANTS YOU THE RIGHTS -CONTAINED HERE IN CONSIDERATION OF YOUR ACCEPTANCE OF SUCH TERMS AND -CONDITIONS. - -1. Definitions - - a. "Adaptation" means a work based upon the Work, or upon the Work and - other pre-existing works, such as a translation, adaptation, - derivative work, arrangement of music or other alterations of a - literary or artistic work, or phonogram or performance and includes - cinematographic adaptations or any other form in which the Work may be - recast, transformed, or adapted including in any form recognizably - derived from the original, except that a work that constitutes a - Collection will not be considered an Adaptation for the purpose of - this License. For the avoidance of doubt, where the Work is a musical - work, performance or phonogram, the synchronization of the Work in - timed-relation with a moving image ("synching") will be considered an - Adaptation for the purpose of this License. - b. "Collection" means a collection of literary or artistic works, such as - encyclopedias and anthologies, or performances, phonograms or - broadcasts, or other works or subject matter other than works listed - in Section 1(f) below, which, by reason of the selection and - arrangement of their contents, constitute intellectual creations, in - which the Work is included in its entirety in unmodified form along - with one or more other contributions, each constituting separate and - independent works in themselves, which together are assembled into a - collective whole. A work that constitutes a Collection will not be - considered an Adaptation (as defined above) for the purposes of this - License. - c. "Distribute" means to make available to the public the original and - copies of the Work or Adaptation, as appropriate, through sale or - other transfer of ownership. - d. "Licensor" means the individual, individuals, entity or entities that - offer(s) the Work under the terms of this License. - e. "Original Author" means, in the case of a literary or artistic work, - the individual, individuals, entity or entities who created the Work - or if no individual or entity can be identified, the publisher; and in - addition (i) in the case of a performance the actors, singers, - musicians, dancers, and other persons who act, sing, deliver, declaim, - play in, interpret or otherwise perform literary or artistic works or - expressions of folklore; (ii) in the case of a phonogram the producer - being the person or legal entity who first fixes the sounds of a - performance or other sounds; and, (iii) in the case of broadcasts, the - organization that transmits the broadcast. - f. "Work" means the literary and/or artistic work offered under the terms - of this License including without limitation any production in the - literary, scientific and artistic domain, whatever may be the mode or - form of its expression including digital form, such as a book, - pamphlet and other writing; a lecture, address, sermon or other work - of the same nature; a dramatic or dramatico-musical work; a - choreographic work or entertainment in dumb show; a musical - composition with or without words; a cinematographic work to which are - assimilated works expressed by a process analogous to cinematography; - a work of drawing, painting, architecture, sculpture, engraving or - lithography; a photographic work to which are assimilated works - expressed by a process analogous to photography; a work of applied - art; an illustration, map, plan, sketch or three-dimensional work - relative to geography, topography, architecture or science; a - performance; a broadcast; a phonogram; a compilation of data to the - extent it is protected as a copyrightable work; or a work performed by - a variety or circus performer to the extent it is not otherwise - considered a literary or artistic work. - g. "You" means an individual or entity exercising rights under this - License who has not previously violated the terms of this License with - respect to the Work, or who has received express permission from the - Licensor to exercise rights under this License despite a previous - violation. - h. "Publicly Perform" means to perform public recitations of the Work and - to communicate to the public those public recitations, by any means or - process, including by wire or wireless means or public digital - performances; to make available to the public Works in such a way that - members of the public may access these Works from a place and at a - place individually chosen by them; to perform the Work to the public - by any means or process and the communication to the public of the - performances of the Work, including by public digital performance; to - broadcast and rebroadcast the Work by any means including signs, - sounds or images. - i. "Reproduce" means to make copies of the Work by any means including - without limitation by sound or visual recordings and the right of - fixation and reproducing fixations of the Work, including storage of a - protected performance or phonogram in digital form or other electronic - medium. - -2. Fair Dealing Rights. Nothing in this License is intended to reduce, -limit, or restrict any uses free from copyright or rights arising from -limitations or exceptions that are provided for in connection with the -copyright protection under copyright law or other applicable laws. - -3. License Grant. Subject to the terms and conditions of this License, -Licensor hereby grants You a worldwide, royalty-free, non-exclusive, -perpetual (for the duration of the applicable copyright) license to -exercise the rights in the Work as stated below: - - a. to Reproduce the Work, to incorporate the Work into one or more - Collections, and to Reproduce the Work as incorporated in the - Collections; - b. to create and Reproduce Adaptations provided that any such Adaptation, - including any translation in any medium, takes reasonable steps to - clearly label, demarcate or otherwise identify that changes were made - to the original Work. For example, a translation could be marked "The - original work was translated from English to Spanish," or a - modification could indicate "The original work has been modified."; - c. to Distribute and Publicly Perform the Work including as incorporated - in Collections; and, - d. to Distribute and Publicly Perform Adaptations. - e. For the avoidance of doubt: - - i. Non-waivable Compulsory License Schemes. In those jurisdictions in - which the right to collect royalties through any statutory or - compulsory licensing scheme cannot be waived, the Licensor - reserves the exclusive right to collect such royalties for any - exercise by You of the rights granted under this License; - ii. Waivable Compulsory License Schemes. In those jurisdictions in - which the right to collect royalties through any statutory or - compulsory licensing scheme can be waived, the Licensor waives the - exclusive right to collect such royalties for any exercise by You - of the rights granted under this License; and, - iii. Voluntary License Schemes. The Licensor waives the right to - collect royalties, whether individually or, in the event that the - Licensor is a member of a collecting society that administers - voluntary licensing schemes, via that society, from any exercise - by You of the rights granted under this License. - -The above rights may be exercised in all media and formats whether now -known or hereafter devised. The above rights include the right to make -such modifications as are technically necessary to exercise the rights in -other media and formats. Subject to Section 8(f), all rights not expressly -granted by Licensor are hereby reserved. - -4. Restrictions. The license granted in Section 3 above is expressly made -subject to and limited by the following restrictions: - - a. You may Distribute or Publicly Perform the Work only under the terms - of this License. You must include a copy of, or the Uniform Resource - Identifier (URI) for, this License with every copy of the Work You - Distribute or Publicly Perform. You may not offer or impose any terms - on the Work that restrict the terms of this License or the ability of - the recipient of the Work to exercise the rights granted to that - recipient under the terms of the License. You may not sublicense the - Work. You must keep intact all notices that refer to this License and - to the disclaimer of warranties with every copy of the Work You - Distribute or Publicly Perform. When You Distribute or Publicly - Perform the Work, You may not impose any effective technological - measures on the Work that restrict the ability of a recipient of the - Work from You to exercise the rights granted to that recipient under - the terms of the License. This Section 4(a) applies to the Work as - incorporated in a Collection, but this does not require the Collection - apart from the Work itself to be made subject to the terms of this - License. If You create a Collection, upon notice from any Licensor You - must, to the extent practicable, remove from the Collection any credit - as required by Section 4(b), as requested. If You create an - Adaptation, upon notice from any Licensor You must, to the extent - practicable, remove from the Adaptation any credit as required by - Section 4(b), as requested. - b. If You Distribute, or Publicly Perform the Work or any Adaptations or - Collections, You must, unless a request has been made pursuant to - Section 4(a), keep intact all copyright notices for the Work and - provide, reasonable to the medium or means You are utilizing: (i) the - name of the Original Author (or pseudonym, if applicable) if supplied, - and/or if the Original Author and/or Licensor designate another party - or parties (e.g., a sponsor institute, publishing entity, journal) for - attribution ("Attribution Parties") in Licensor's copyright notice, - terms of service or by other reasonable means, the name of such party - or parties; (ii) the title of the Work if supplied; (iii) to the - extent reasonably practicable, the URI, if any, that Licensor - specifies to be associated with the Work, unless such URI does not - refer to the copyright notice or licensing information for the Work; - and (iv) , consistent with Section 3(b), in the case of an Adaptation, - a credit identifying the use of the Work in the Adaptation (e.g., - "French translation of the Work by Original Author," or "Screenplay - based on original Work by Original Author"). The credit required by - this Section 4 (b) may be implemented in any reasonable manner; - provided, however, that in the case of a Adaptation or Collection, at - a minimum such credit will appear, if a credit for all contributing - authors of the Adaptation or Collection appears, then as part of these - credits and in a manner at least as prominent as the credits for the - other contributing authors. For the avoidance of doubt, You may only - use the credit required by this Section for the purpose of attribution - in the manner set out above and, by exercising Your rights under this - License, You may not implicitly or explicitly assert or imply any - connection with, sponsorship or endorsement by the Original Author, - Licensor and/or Attribution Parties, as appropriate, of You or Your - use of the Work, without the separate, express prior written - permission of the Original Author, Licensor and/or Attribution - Parties. - c. Except as otherwise agreed in writing by the Licensor or as may be - otherwise permitted by applicable law, if You Reproduce, Distribute or - Publicly Perform the Work either by itself or as part of any - Adaptations or Collections, You must not distort, mutilate, modify or - take other derogatory action in relation to the Work which would be - prejudicial to the Original Author's honor or reputation. Licensor - agrees that in those jurisdictions (e.g. Japan), in which any exercise - of the right granted in Section 3(b) of this License (the right to - make Adaptations) would be deemed to be a distortion, mutilation, - modification or other derogatory action prejudicial to the Original - Author's honor and reputation, the Licensor will waive or not assert, - as appropriate, this Section, to the fullest extent permitted by the - applicable national law, to enable You to reasonably exercise Your - right under Section 3(b) of this License (right to make Adaptations) - but not otherwise. - -5. Representations, Warranties and Disclaimer - -UNLESS OTHERWISE MUTUALLY AGREED TO BY THE PARTIES IN WRITING, LICENSOR -OFFERS THE WORK AS-IS AND MAKES NO REPRESENTATIONS OR WARRANTIES OF ANY -KIND CONCERNING THE WORK, EXPRESS, IMPLIED, STATUTORY OR OTHERWISE, -INCLUDING, WITHOUT LIMITATION, WARRANTIES OF TITLE, MERCHANTIBILITY, -FITNESS FOR A PARTICULAR PURPOSE, NONINFRINGEMENT, OR THE ABSENCE OF -LATENT OR OTHER DEFECTS, ACCURACY, OR THE PRESENCE OF ABSENCE OF ERRORS, -WHETHER OR NOT DISCOVERABLE. SOME JURISDICTIONS DO NOT ALLOW THE EXCLUSION -OF IMPLIED WARRANTIES, SO SUCH EXCLUSION MAY NOT APPLY TO YOU. - -6. Limitation on Liability. EXCEPT TO THE EXTENT REQUIRED BY APPLICABLE -LAW, IN NO EVENT WILL LICENSOR BE LIABLE TO YOU ON ANY LEGAL THEORY FOR -ANY SPECIAL, INCIDENTAL, CONSEQUENTIAL, PUNITIVE OR EXEMPLARY DAMAGES -ARISING OUT OF THIS LICENSE OR THE USE OF THE WORK, EVEN IF LICENSOR HAS -BEEN ADVISED OF THE POSSIBILITY OF SUCH DAMAGES. - -7. Termination - - a. This License and the rights granted hereunder will terminate - automatically upon any breach by You of the terms of this License. - Individuals or entities who have received Adaptations or Collections - from You under this License, however, will not have their licenses - terminated provided such individuals or entities remain in full - compliance with those licenses. Sections 1, 2, 5, 6, 7, and 8 will - survive any termination of this License. - b. Subject to the above terms and conditions, the license granted here is - perpetual (for the duration of the applicable copyright in the Work). - Notwithstanding the above, Licensor reserves the right to release the - Work under different license terms or to stop distributing the Work at - any time; provided, however that any such election will not serve to - withdraw this License (or any other license that has been, or is - required to be, granted under the terms of this License), and this - License will continue in full force and effect unless terminated as - stated above. - -8. Miscellaneous - - a. Each time You Distribute or Publicly Perform the Work or a Collection, - the Licensor offers to the recipient a license to the Work on the same - terms and conditions as the license granted to You under this License. - b. Each time You Distribute or Publicly Perform an Adaptation, Licensor - offers to the recipient a license to the original Work on the same - terms and conditions as the license granted to You under this License. - c. If any provision of this License is invalid or unenforceable under - applicable law, it shall not affect the validity or enforceability of - the remainder of the terms of this License, and without further action - by the parties to this agreement, such provision shall be reformed to - the minimum extent necessary to make such provision valid and - enforceable. - d. No term or provision of this License shall be deemed waived and no - breach consented to unless such waiver or consent shall be in writing - and signed by the party to be charged with such waiver or consent. - e. This License constitutes the entire agreement between the parties with - respect to the Work licensed here. There are no understandings, - agreements or representations with respect to the Work not specified - here. Licensor shall not be bound by any additional provisions that - may appear in any communication from You. This License may not be - modified without the mutual written agreement of the Licensor and You. - f. The rights granted under, and the subject matter referenced, in this - License were drafted utilizing the terminology of the Berne Convention - for the Protection of Literary and Artistic Works (as amended on - September 28, 1979), the Rome Convention of 1961, the WIPO Copyright - Treaty of 1996, the WIPO Performances and Phonograms Treaty of 1996 - and the Universal Copyright Convention (as revised on July 24, 1971). - These rights and subject matter take effect in the relevant - jurisdiction in which the License terms are sought to be enforced - according to the corresponding provisions of the implementation of - those treaty provisions in the applicable national law. If the - standard suite of rights granted under applicable copyright law - includes additional rights not granted under this License, such - additional rights are deemed to be included in the License; this - License is not intended to restrict the license of any rights under - applicable law. - - -Creative Commons Notice - - Creative Commons is not a party to this License, and makes no warranty - whatsoever in connection with the Work. Creative Commons will not be - liable to You or any party on any legal theory for any damages - whatsoever, including without limitation any general, special, - incidental or consequential damages arising in connection to this - license. Notwithstanding the foregoing two (2) sentences, if Creative - Commons has expressly identified itself as the Licensor hereunder, it - shall have all rights and obligations of Licensor. - - Except for the limited purpose of indicating to the public that the - Work is licensed under the CCPL, Creative Commons does not authorize - the use by either party of the trademark "Creative Commons" or any - related trademark or logo of Creative Commons without the prior - written consent of Creative Commons. Any permitted use will be in - compliance with Creative Commons' then-current trademark usage - guidelines, as may be published on its website or otherwise made - available upon request from time to time. For the avoidance of doubt, - this trademark restriction does not form part of this License. - - Creative Commons may be contacted at https://creativecommons.org/. diff --git a/README.md b/README.md new file mode 100644 index 0000000..f244226 --- /dev/null +++ b/README.md @@ -0,0 +1,33 @@ +# Google Engineering Practices Documentation + +Google has many generalized engineering practices that cover all languages and +all projects. These documents represent our collective experience of various +best practices that we have developed over time. It is possible that open source +projects or other organizations would benefit from this knowledge, so we work to +make it available publicly when possible. + +Currently this contains the following documents: + +* [Google's Code Review Guidelines](review/index.md), which are actually two + separate sets of documents: + * [The Code Reviewer's Guide](review/reviewer/index.md) + * [The Change Author's Guide](review/developer/index.md) + +## Terminology + +There is some Google-internal terminology used in some of these documents, which +we clarify here for external readers: + +* **CL**: Stands for "changelist," which means one self-contained change that + has been submitted to version control or which is undergoing code review. + Other organizations often call this a "change" or a "patch." +* **LGTM**: Means "Looks Good to Me." It is what a code reviewer says when + approving a CL. + +## License + +The documents in this project are licensed under the CC-By 3.0 License, which +encourages you to share these documents. See +https://creativecommons.org/licenses/by/3.0/ for more details. + +Creative Commons License diff --git a/review/developer/cl-descriptions.md b/review/developer/cl-descriptions.md new file mode 100644 index 0000000..2975d7d --- /dev/null +++ b/review/developer/cl-descriptions.md @@ -0,0 +1,119 @@ +# Writing good CL descriptions + +[TOC] + +A CL description is a public record of **what** change is being made and **why** +it was made. It will become a permanent part of our version control history, and +will possibly be read by hundreds of people other than your reviewers over the +years. + +Future developers will search for your CL based on its description. Someone in +the future might be looking for your change because of a faint memory of its +relevance but without the specifics handy. If all the important information is +in the code and not the description, it's going to be a lot harder for them to +locate your CL. + +## First Line {#firstline} + +* Short summary of what is being done. +* Complete sentence, written as though it was an order. +* Follow by empty line. + +The **first line** of a CL description should be a short summary of +*specifically* **what** *is being done by the CL*, followed by a blank line. +This is what most future code searchers will see when they are browsing the +version control history of a piece of code, so this first line should be +informative enough that they don't have to read your CL or its whole description +just to get a general idea of what your CL actually *did*. + +By tradition, the first line of a CL description is a complete sentence, written +as though it were an order (an imperative sentence). For example, say +\"**Delete** the FizzBuzz RPC and **replace** it with the new system." instead +of \"**Deleting** the FizzBuzz RPC and **replacing** it with the new system." +You don't have to write the rest of the description as an imperative sentence, +though. + +## Body is Informative {#informative} + +The rest of the description should be informative. It might include a brief +description of the problem that's being solved, and why this is the best +approach. If there are any shortcomings to the approach, they should be +mentioned. If relevant, include background information such as bug numbers, +benchmark results, and links to design documents. + +Even small CLs deserve a little attention to detail. Put the CL in context. + +## Bad CL Descriptions {#bad} + +"Fix bug" is an inadequate CL description. What bug? What did you do to fix it? +Other similarly bad descriptions include: + +- "Fix build." +- "Add patch." +- "Moving code from A to B." +- "Phase 1." +- "Add convenience functions." +- "kill weird URLs." + +Some of those are real CL descriptions. Their authors may believe they are +providing useful information, but they are not serving the purpose of a CL +description. + +## Good CL Descriptions {#good} + +Here are some examples of good descriptions. + +### Functionality change + +> rpc: remove size limit on RPC server message freelist. +> +> Servers like FizzBuzz have very large messages and would benefit from reuse. +> Make the freelist larger, and add a goroutine that frees the freelist entries +> slowly over time, so that idle servers eventually release all freelist +> entries. + +The first few words describe what the CL actually does. The rest of the +description talks about the problem being solved, why this is a good solution, +and a bit more information about the specific implementation. + +### Refactoring + +> Construct a Task with a TimeKeeper to use its TimeStr and Now methods. +> +> Add a Now method to Task, so the borglet() getter method can be removed (which +> was only used by OOMCandidate to call borglet's Now method). This replaces the +> methods on Borglet that delegate to a TimeKeeper. +> +> Allowing Tasks to supply Now is a step toward eliminating the dependency on +> Borglet. Eventually, collaborators that depend on getting Now from the Task +> should be changed to use a TimeKeeper directly, but this has been an +> accommodation to refactoring in small steps. +> +> Continuing the long-range goal of refactoring the Borglet Hierarchy. + +The first line describes what the CL does and how this is a change from the +past. The rest of the description talks about the specific implementation, the +context of the CL, that the solution isn't ideal, and possible future direction. +It also explains *why* this change is being made. + +### Small CL that needs some context + +> Create a Python3 build rule for status.py. +> +> This allows consumers who are already using this as in Python3 to depend on a +> rule that is next to the original status build rule instead of somewhere in +> their own tree. It encourages new consumers to use Python3 if they can, +> instead of Python2, and significantly simplifies some automated build file +> refactoring tools being worked on currently. + +The first sentence describes what's actually being done. The rest of the +description explains *why* the change is being made and gives the reviewer a lot +of context. + +## Review the description before submitting the CL + +CLs can undergo significant change during review. It can be worthwhile to review +a CL description before submitting the CL, to ensure that the description still +reflects what the CL does. + +Next: [Small CLs](small-cls.md) diff --git a/review/developer/handling-comments.md b/review/developer/handling-comments.md new file mode 100644 index 0000000..9bcc6be --- /dev/null +++ b/review/developer/handling-comments.md @@ -0,0 +1,78 @@ +# How to handle reviewer comments + +[TOC] + +When you've sent a CL out for review, it's likely that your reviewer will +respond with several comments on your CL. Here are some useful things to know +about handling reviewer comments. + +## Don't Take it Personally {#personal} + +The goal of review is to maintain the quality of our codebase and our products. +When a reviewer provides a critique of your code, think of it as their attempt +to help you, the codebase, and Google, rather than as a personal attack on you +or your abilities. + +Sometimes reviewers feel frustrated and they express that frustration in their +comments. This isn't a good practice for reviewers, but as a developer you +should be prepared for this. Ask yourself, "What is the constructive thing that +the reviewer is trying to communicate to me?" and then operate as though that's +what they actually said. + +**Never respond in anger to code review comments.** That is a serious breach of +professional etiquette that will live forever in the code review tool. If you +are too angry or annoyed to respond kindly, then walk away from your computer +for a while, or work on something else until you feel calm enough to reply +politely. + +In general, if a reviewer isn't providing feedback in a way that's constructive +and polite, explain this to them in person. If you can't talk to them in person +or on a video call, then send them a private email. Explain to them in a kind +way what you don't like and what you'd like them to do differently. If they also +respond in a non-constructive way to this private discussion, or it doesn't have +the intended effect, then +escalate to your manager as +appropriate. + +## Fix the Code {#code} + +If a reviewer says that they don't understand something in your code, your first +response should be to clarify the code itself. If the code can't be clarified, +add a code comment that explains why the code is there. If a comment seems +pointless, only then should your response be an explanation in the code review +tool. + +If a reviewer didn't understand some piece of your code, it's likely other +future readers of the code won't understand either. Writing a response in the +code review tool doesn't help future code readers, but clarifying your code or +adding code comments does help them. + +## Think for Yourself {#think} + +Writing a CL can take a lot of work. It's often really satisfying to finally +send one out for review, feel like it's done, and be pretty sure that no further +work is needed. So when a reviewer comes back with comments on things that could +be improved, it's easy to reflexively think the comments are wrong, the reviewer +is blocking you unnecessarily, or they should just let you submit the CL. +However, **no matter how certain you are** at this point, take a moment to step +back and consider if the reviewer is providing valuable feedback that will help +the codebase and Google. Your first question to yourself should always be, "Is +the reviewer correct?" + +If you can't answer that question, it's likely the reviewer needs to clarify +their comments. + +If you *have* considered it and you still think you're right, feel free to +respond with an explanation of why your method of doing things is better for the +codebase, users, and/or Google. Often, reviewers are actually providing +*suggestions* and they want you to think for yourself about what's best. You +might actually know something about the users, codebase, or CL that the reviewer +doesn't know. So fill them in; give them more context. Usually you can come to +some consensus between yourself and the reviewer based on technical facts. + +## Resolving Conflicts {#conflicts} + +Your first step in resolving conflicts should always be to try to come to +consensus with your reviewer. If you can't achieve consensus, see +[The Standard of Code Review](../reviewer/standard.md), which gives principles +to follow in such a situation. diff --git a/review/developer/index.md b/review/developer/index.md new file mode 100644 index 0000000..6538d36 --- /dev/null +++ b/review/developer/index.md @@ -0,0 +1,16 @@ +# The CL author's guide to getting through code review + +go/cl-author + +The pages in this section contain best practices for developers going through +code review. These guidelines should help you get through reviews faster and +with higher-quality results. You don't have to read them all, but they are +intended to apply to every Google developer, and many people have found it +helpful to read the whole set. + +- [Writing Good CL Descriptions](cl-descriptions.md) +- [Small CLs](small-cls.md) +- [How to Handle Reviewer Comments](handling-comments.md) + +See also [How to Do a Code Review](../reviewer/), which gives detailed guidance +for code reviewers. diff --git a/review/developer/small-cls.md b/review/developer/small-cls.md new file mode 100644 index 0000000..c5ac41a --- /dev/null +++ b/review/developer/small-cls.md @@ -0,0 +1,153 @@ +# Small CLs + +[TOC] + +## Why Write Small CLs? {#why} + +Small, simple CLs are: + +- **Reviewed more quickly.** It's easier for a reviewer to find five minutes + several times to review small CLs than to set aside a 30 minute block to + review one large CL. +- **Reviewed more thoroughly.** With large changes, reviewers and authors tend + to get frustrated by large volumes of detailed commentary shifting back and + forth—sometimes to the point where important points get missed or dropped. +- **Less likely to introduce bugs.** Since you're making fewer changes, it's + easier for you and your reviewer to reason effectively about the impact of + the CL and see if a bug has been introduced. +- **Less wasted work if they are rejected.** If you write a huge CL and then + your reviewer says that the overall direction is wrong, you've wasted a lot + of work. +- **Easier to merge.** Working on a large CL takes a long time, so you will + have lots of conflicts when you merge, and you will have to merge + frequently. +- **Easier to design well.** It's a lot easier to polish the design and code + health of a small change than it is to refine all the details of a large + change. +- **Less blocking on reviews.** Sending self-contained portions of your + overall change allows you to continue coding while you wait for your current + CL in review. +- **Simpler to roll back.** A large CL will more likely touch files that get + updated between the initial CL submission and a rollback CL, complicating + the rollback (the intermediate CLs will probably need to be rolled back + too). + +Note that **reviewers have discretion to reject your change outright for the +sole reason of it being too large.** Usually they will thank you for your +contribution but request that you somehow make it into a series of smaller +changes. It can be a lot of work to split up a change after you've already +written it, or require lots of time arguing about why the reviewer should accept +your large change. It's easier to just write small CLs in the first place. + +## What is Small? {#what_is_small} + +In general, the right size for a CL is **one self-contained change**. This means +that: + +- The CL makes a minimal change that addresses **just one thing**. This is + usually just one part of a feature, rather than a whole feature at once. In + general it's better to err on the side of writing CLs that are too small vs. + CLs that are too large. Work with your reviewer to find out what an + acceptable size is. +- Everything the reviewer needs to understand about the CL (except future + development) is in the CL, the CL's description, the existing codebase, or a + CL they've already reviewed. +- The system will continue to work well for its users and for the developers + after the CL is checked in. +- The CL is not so small that its implications are difficult to understand. If + you add a new API, you should include a usage of the API in the same CL so + that reviewers can better understand how the API will be used. This also + prevents checking in unused APIs. + +There are no hard and fast rules about how large is "too large." 100 lines is +usually a reasonable size for a CL, and 1000 lines is usually too large, but +it's up to the judgment of your reviewer. The number of files that a change is +spread across also affects its "size." A 200-line change in one file might be +okay, but spread across 50 files it would usually be too large. + +Keep in mind that although you have been intimately involved with your code from +the moment you started to write it, the reviewer often has no context. What +seems like an acceptably-sized CL to you might be overwhelming to your reviewer. +When in doubt, write CLs that are smaller than you think you need to write. +Reviewers rarely complain about getting CLs that are too small. + +## When are Large CLs Okay? {#large_okay} + +There are a few situations in which large changes aren't as bad: + +- You can usually count deletion of an entire file as being just one line of + change, because it doesn't take the reviewer very long to review. +- Sometimes a large CL has been generated by an automatic refactoring tool + that you trust completely, and the reviewer's job is just to sanity check + and say that they really do want the change. These CLs can be larger, + although some of the caveats from above (such as merging and testing) still + apply. + +### Splitting by Files {#splitting-files} + +Another way to split up a CL is by groupings of files that will require +different reviewers but are otherwise self-contained changes. + +For example: you send off one CL for modifications to a protocol buffer and +another CL for changes to the code that uses that proto. You have to submit the +proto CL before the code CL, but they can both be reviewed simultaneously. If +you do this, you might want to inform both sets of reviewers about the other CL +that you wrote, so that they have context for your changes. + +Another example: you send one CL for a code change and another for the +configuration or experiment that uses that code; this is easier to roll back +too, if necessary, as configuration/experiment files are sometimes pushed to +production faster than code changes. + +## Separate Out Refactorings {#refactoring} + +It's usually best to do refactorings in a separate CL from feature changes or +bug fixes. For example, moving and renaming a class should be in a different CL +from fixing a bug in that class. It is much easier for reviewers to understand +the changes introduced by each CL when they are separate. + +Small cleanups such as fixing a local variable name can be included inside of a +feature change or bug fix CL, though. It's up to the judgment of developers and +reviewers to decide when a refactoring is so large that it will make the review +more difficult if included in your current CL. + +## Keep related test code in the same CL {#test_code} + +Avoid splitting test code into a separate CL. Tests validating your code +modifications should go into the same CL, even if it increases the code line +count. + +However, independent test modifications can go into separate CLs first, +similar to the [refactorings guidelines](#refactoring). That includes: + +* validating pre-existing, submitted code with new tests. +* refactoring the test code (e.g. introduce helper functions). +* introducing larger test framework code (e.g. an integration test). + +## Don't Break the Build {#break} + +If you have several CLs that depend on each other, you need to find a way to +make sure the whole system keeps working after each CL is submitted. Otherwise +you might break the build for all your fellow developers for a few minutes +between your CL submissions (or even longer if something goes wrong unexpectedly +with your later CL submissions). + +## Can't Make it Small Enough {#cant} + +Sometimes you will encounter situations where it seems like your CL *has* to be +large. This is very rarely true. Authors who practice writing small CLs can +almost always find a way to decompose functionality into a series of small +changes. + +Before writing a large CL, consider whether preceding it with a refactoring-only +CL could pave the way for a cleaner implementation. Talk to your teammates and +see if anybody has thoughts on how to implement the functionality in small CLs +instead. + +If all of these options fail (which should be extremely rare) then get consent +from your reviewers in advance to review a large CL, so they are warned about +what is coming. In this situation, expect to be going through the review process +for a long time, be vigilant about not introducing bugs, and be extra diligent +about writing tests. + +Next: [How to Handle Reviewer Comments](handling-comments.md) diff --git a/review/emergencies.md b/review/emergencies.md new file mode 100644 index 0000000..580c34c --- /dev/null +++ b/review/emergencies.md @@ -0,0 +1,69 @@ +# Emergencies + +Sometimes there are emergency CLs that must pass through the entire code review +process as quickly as +possible. + +[TOC] + +## What Is An Emergency? {#what} + +An emergency CL would be a **small** change that: allows a major launch to +continue instead of rolling back, fixes a bug significantly affecting users in +production, handles a pressing legal issue, closes a major security hole, etc. + +In emergencies we really do care about the speed of the entire code review +process, not just the [speed of response](reviewer/speed.md). In this case +*only*, the reviewer should care more about the speed of the review and the +correctness of the code (does it actually resolve the emergency?) than anything +else. Also (perhaps obviously) such reviews should take priority over all other +code reviews, when they come up. + +However, after the emergency is resolved you should look over the emergency CLs +again and give them a [more thorough review](reviewer/looking-for.md). + +## What Is Not An Emergency? {#not} + +To be clear, the following cases are *not* an emergency: + +- Wanting to launch this week rather than next week (unless there is some + actual [hard deadline](#deadlines) for launch such as a partner agreement). +- The developer has worked on a feature for a very long time and they really + want to get the CL in. +- The reviewers are all in another timezone where it is currently nighttime or + they are away on an off-site. +- It is the end of the day on a Friday and it would just be great to get this + CL in before the developer leaves for the weekend. +- A manager says that this review has to be complete and the CL checked in + today because of a [soft (not hard) deadline](#deadlines). +- Rolling back a CL that is causing test failures or build breakages. + +And so on. + +## What Is a Hard Deadline? {#deadlines} + +A hard deadline is one where **something disastrous would happen** if you miss +it. For example: + +- Submitting your CL by a certain date is necessary for a contractual + obligation. +- Your product will completely fail in the marketplace if not released by a + certain date. +- Some hardward manufacturers only ship new hardware once a year. If you miss + the deadline to submit code to them, that could be disastrous, depending on + what type of code you’re trying to ship. + +Delaying a release for a week is not disastrous. Missing an important conference +might be disastrous, but often is not. + +Most deadlines are soft deadlines, not hard deadlines. They represent a desire +for a feature to be done by a certain time. They are important, but you +shouldn’t be sacrificing code health to make them. + +If you have a long release cycle (several weeks) it can be tempting to sacrifice +code review quality to get a feature in before the next cycle. However, this +pattern, if repeated, is a common way for projects to build up overwhelming +technical debt. If developers are routinely submitting CLs near the end of the +cycle that "must get in" with only superficial review, then the team should +modify its process so that large feature changes happen early in the cycle and +have enough time for good review. diff --git a/review/index.md b/review/index.md new file mode 100644 index 0000000..658e8f0 --- /dev/null +++ b/review/index.md @@ -0,0 +1,69 @@ +# Code Review Developer Guide + +## Introduction {#intro} + +A code review is a process where someone other than the author(s) of a piece of +code examines that code. + +At Google we use code review to maintain the quality of our code and products. + +This documentation is the canonical description of Google's code review +processes and policies. + +[TOC] + +This page is an overview of our code review process. There are two other large +documents that are a part of this guide: + +- **[How To Do A Code Review](reviewer/)**: A detailed guide for code + reviewers. +- **[The CL Author's Guide](developer/)**: A detailed guide for developers + whose CLs are going through review. + +## What Do Code Reviewers Look For? {#look_for} + +Code reviews should look at: + +- **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](http://google.github.io/styleguide/)? +- **Documentation**: Did the developer also update relevant documentation? + +See **[How To Do A Code Review](reviewer/)** for more information. + +### Picking the Best Reviewers {#best_reviewers} + +In general, you want to find the *best* reviewers you can who are capable of +responding to your review within a reasonable period of time. + +The best reviewer is the person who will be able to give you the most thorough +and correct review for the piece of code you are writing. This usually means the +owner(s) of the code, who may or may not be the people in the OWNERS file. +Sometimes this means asking different people to review different parts of the +CL. + +If you find an ideal reviewer but they are not available, you should at least CC +them on your change. + +### In-Person Reviews {#in_person} + +If you pair-programmed a piece of code with somebody who was qualified to do a +good code review on it, then that code is considered reviewed. + +You can also do in-person code reviews where the reviewer asks questions and the +developer of the change speaks only when spoken to. + +## See Also {#seealso} + +- [How To Do A Code Review](reviewer/): A detailed guide for code reviewers. +- [The CL Author's Guide](developer/): A detailed guide for developers whose + CLs are going through review. diff --git a/review/reviewer/comments.md b/review/reviewer/comments.md new file mode 100644 index 0000000..1a316c6 --- /dev/null +++ b/review/reviewer/comments.md @@ -0,0 +1,70 @@ +# How to write code review comments + +[TOC] + +## Summary + +- Be kind. +- Explain your reasoning. +- Balance giving explicit directions with just pointing out problems and + letting the developer decide. +- Encourage developers to simplify code or add code comments instead of just + explaining the complexity to you. + +## Courtesy + +In general, it is important to be +courteous and respectful while also being +very clear and helpful to the developer whose code you are reviewing. One way to +do this is to be sure that you are always making comments about the *code* and +never making comments about the *developer*. You don't always have to follow +this practice, but you should definitely use it when saying something that might +otherwise be upsetting or contentious. For example: + +Bad: "Why did **you** use threads here when there's obviously no benefit to be +gained from concurrency?" + +Good: "The concurrency model here is adding complexity to the system without any +actual performance benefit that I can see. Because there's no performance +benefit, it's best for this code to be single-threaded instead of using multiple +threads." + +## Explain Why {#why} + +One thing you'll notice about the "good" example from above is that it helps the +developer understand *why* you are making your comment. You don't always need to +include this information in your review comments, but sometimes it's appropriate +to give a bit more explanation around your intent, the best practice you're +following, or how your suggestion improves code health. + +## Giving Guidance {#guidance} + +**In general it is the developer's responsibility to fix a CL, not the +reviewer's.** You are not required to do detailed design of a solution or write +code for the developer. + +This doesn't mean the reviewer should be unhelpful, though. In general you +should strike an appropriate balance between pointing out problems and providing +direct guidance. Pointing out problems and letting the developer make a decision +often helps the developer learn, and makes it easier to do code reviews. It also +can result in a better solution, because the developer is closer to the code +than the reviewer is. + +However, sometimes direct instructions, suggestions, or even code are more +helpful. The primary goal of code review is to get the best CL possible. A +secondary goal is improving the skills of developers so that they require less +and less review over time. + +## Accepting Explanations {#explanations} + +If you ask a developer to explain a piece of code that you don't understand, +that should usually result in them **rewriting the code more clearly**. +Occasionally, adding a comment in the code is also an appropriate response, as +long as it's not just explaining overly complex code. + +**Explanations written only in the code review tool are not helpful to future +code readers.** They are acceptable only in a few circumstances, such as when +you are reviewing an area you are not very familiar with and the developer +explains something that normal readers of the code would have already known. + +Next: [Handling Pushback in Code Reviews](pushback.md) diff --git a/review/reviewer/index.md b/review/reviewer/index.md new file mode 100644 index 0000000..2ce3d7d --- /dev/null +++ b/review/reviewer/index.md @@ -0,0 +1,19 @@ +# How to do a code review + +go/code-reviewer + +The pages in this section contain recommendations on the best way to do code +reviews, based on long experience. All together they represent one complete +document, broken up into many separate sections. You don't have to read them +all, but many people have found it very helpful to themselves and their team to +read the entire set. + +- [The Standard of Code Review](standard.md) +- [What to Look For In a Code Review](looking-for.md) +- [Navigating a CL in Review](navigate.md) +- [Speed of Code Reviews](speed.md) +- [How to Write Code Review Comments](comments.md) +- [Handling Pushback in Code Reviews](pushback.md) + +See also the [CL Author's Guide](../developer/), which gives detailed guidance +to developers whose CLs are undergoing review. diff --git a/review/reviewer/looking-for.md b/review/reviewer/looking-for.md new file mode 100644 index 0000000..c42b582 --- /dev/null +++ b/review/reviewer/looking-for.md @@ -0,0 +1,204 @@ +# What to look for in a code review + +[TOC] + +Note: Always make sure to take into account +[The Standard of Code Review](standard.md) when considering each of these +points. + +## Design + +The most important thing to cover in a review is the overall design of the CL. +Do the interactions of various pieces of code in the CL make sense? Does this +change belong in your codebase, or in a library? Does it integrate well with the +rest of your system? Is now a good time to add this functionality? + +## Functionality + +Does this CL do what the developer intended? Is what the developer intended good +for the users of this code? The "users" are usually both end-users (when they +are affected by the change) and developers (who will have to "use" this code in +the future). + +Mostly, we expect developers to test CLs well-enough that they work correctly by +the time they get to code review. However, as the reviewer you should still be +thinking about edge cases, looking for concurrency problems, trying to think +like a user, and making sure that there are no bugs that you see just by reading +the code. + +You *can* validate the CL if you want—the time when it's most important for a +reviewer to check a CL's behavior is when it has a user-facing impact, such as a +**UI change**. It's hard to understand how some changes will impact a user when +you're just reading the code. 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. + +Another time when it's particularly important to think about functionality +during a code review is if there is some sort of **parallel programming** going +on in the CL that could theoretically cause deadlocks or race conditions. These +sorts of issues are very hard to detect by just running the code and usually +need somebody (both the developer and the reviewer) to think through them +carefully to be sure that problems aren't being introduced. (Note that this is +also a good reason not to use concurrency models where race conditions or +deadlocks are possible—it can make it very complex to do code reviews or +understand the code.) + +## Complexity + +Is the CL more complex than it should be? Check this at every level of the +CL—are individual lines too complex? Are functions too complex? Are classes too +complex? "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."** + +A particular type of complexity is **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. Reviewers should be especially vigilant +about over-engineering. Encourage developers to 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. + +## Tests + +Ask for unit, integration, or end-to-end +tests as appropriate for the change. In general, tests should be added in the +same CL as the production code unless the CL is handling an +[emergency](../emergencies.shtml). + +Make sure that the tests in the CL are correct, sensible, and useful. Tests do +not test themselves, and we rarely write tests for our tests—a human must ensure +that tests are valid. + +Will the tests actually fail when the code is broken? If the code changes +beneath them, will they start producing false positives? Does each test make +simple and useful assertions? Are the tests separated appropriately between +different test methods? + +Remember that tests are also code that has to be maintained. Don't accept +complexity in tests just because they aren't part of the main binary. + +## Naming + +Did the developer pick good names for everything? 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. + +## Comments + +Did the developer write clear comments in understandable English? Are all of the +comments actually necessary? Usually comments are useful when they **explain +why** some code exists, and should not be explaining *what* some code is doing. +If the code isn't clear enough to explain itself, then the code should be made +simpler. There are some exceptions (regular expressions and complex algorithms +often benefit greatly from comments that explain what they're doing, for +example) but mostly comments are for information that the code itself can't +possibly contain, like the reasoning behind a decision. + +It can also be helpful to look at comments that were there before this CL. Maybe +there is a TODO that can be removed now, a comment advising against this change +being made, etc. + +Note that comments are different from *documentation* of classes, modules, or +functions, which should instead express the purpose of a piece of code, how it +should be used, and how it behaves when used. + +## Style + +We have [style guides](http://google.github.io/styleguide/) at Google for all +of our major languages, and even for most of the minor languages. Make sure the +CL follows the appropriate style guides. + +If you want to improve some style point that isn't in the style guide, prefix +your comment with "Nit:" to let the developer know that it's a nitpick that you +think would improve the code but isn't mandatory. Don't block CLs from being +submitted based only on personal style preferences. + +The author of the CL should not include major style changes combined with other +changes. It makes it hard to see what is being changed in the CL, makes merges +and rollbacks more complex, and causes other problems. For example, if the +author wants to reformat the whole file, have them send you just the +reformatting to as one CL, and then send another CL with their functional +changes after that. + +## Documentation + +If a CL changes how users build, test, interact with, or release code, check to +see that it also updates associated documentation, including +READMEs, g3doc pages, and any generated +reference docs. If the CL deletes or deprecates code, consider whether the +documentation should also be deleted. +If documentation is +missing, ask for it. + +## Every Line {#every_line} + +Look at *every* line of code that you have been assigned to review. Some things +like data files, generated code, or large data structures you can scan over +sometimes, but don't scan over a human-written class, function, or block of code +and assume that what's inside of it is okay. Obviously some code deserves more +careful scrutiny than other code—that's a judgment call that you have to +make—but you should at least be sure that you *understand* what all the +code is doing. + +If it's too hard for you to read the code and this is slowing down the review, +then you should let the developer know that +and wait for them to clarify it before you try to review it. At Google, we hire +great software engineers, and you are one of them. If you can't understand the +code, it's very likely that other developers won't either. So you're also +helping future developers understand this code, when you ask the developer to +clarify it. + +If you understand the code but you don't feel qualified to do some part of the +review, make sure there is a reviewer on the CL who is qualified, particularly +for complex issues such as security, concurrency, accessibility, +internationalization, etc. + +## Context + +It is often helpful to look at the CL in a broad context. Usually the code +review tool will only show you a few lines of code around the parts that are +being changed. Sometimes you have to look at the whole file to be sure that the +change actually makes sense. For example, you might see only four new lines +being added, but when you look at the whole file, you see those four lines are +in a 50-line method that now really needs to be broken up into smaller methods. + +It's also useful to think about the CL in the context of the system as a whole. +Is this CL improving the code health of the system or is it making the whole +system more complex, less tested, etc.? **Don't accept CLs that degrade the code +health of the system.** Most systems become complex through many small changes +that add up, so it's important to prevent even small complexities in new +changes. + +## Good Things {#good_things} + +If you see something nice in the CL, tell the developer, especially when they +addressed one of your comments in a great way. Code reviews often just focus on +mistakes, but they should offer encouragement and appreciation for good +practices, as well. It’s sometimes even more valuable, in terms of mentoring, to +tell a developer what they did right than to tell them what they did wrong. + +## Summary + +In doing a code review, you should make sure that: + +- The code is well-designed. +- The functionality is good for the users of the code. +- Any UI changes are sensible and look good. +- Any parallel programming is done safely. +- The code isn't more complex than it needs to be. +- The developer isn't implementing things they *might* need in the future but + don't know they need now. +- Code has appropriate unit tests. +- Tests are well-designed. +- The developer used clear names for everything. +- Comments are clear and useful, and mostly explain *why* instead of *what*. +- Code is appropriately documented (generally in g3doc). +- The code conforms to our style guides. + +Make sure to review **every line** of code you've been asked to review, look at +the **context**, make sure you're **improving code health**, and compliment +developers on **good things** that they do. + +Next: [Navigating a CL in Review](navigate.md) diff --git a/review/reviewer/navigate.md b/review/reviewer/navigate.md new file mode 100644 index 0000000..98acf9c --- /dev/null +++ b/review/reviewer/navigate.md @@ -0,0 +1,79 @@ +# Navigating a CL in review + +[TOC] + +## Summary + +Now that you know [what to look for](looking-for.md), what's the most efficient +way to manage a review that's spread across multiple files? + +1. Does the change make sense? Does it have a good + [description](../developer/cl-descriptions.md)? +2. Look at the most important part of the change first. Is it well-designed + overall? +3. Look at the rest of the CL in an appropriate sequence. + +## Step One: Take a broad view of the change {#step_one} + +Look at the [CL description](../developer/cl-descriptions.md) and what the CL +does in general. Does this change even make sense? If this change shouldn't have +happened in the first place, please respond immediately with an explanation of +why the change should not be happening. When you reject a change like this, it's +also a good idea to suggest to the developer what they should have done instead. + +For example, you might say "Looks like you put some good work into this, thanks! +However, we're actually going in the direction of removing the FooWidget system +that you're modifying here, and so we don't want to make any new modifications +to it right now. How about instead you refactor our new BarWidget class?" + +Note that not only did the reviewer reject the current CL and provide an +alternative suggestion, but they did it *courteously*. This kind of courtesy is +important because we want to show that we respect each other as developers even +when we disagree. + +If you get more than a few CLs that represent changes you don't want to make, +you should consider re-working your team's development process or the posted +process for external contributors so that there is more communication before CLs +are written. It's better to tell people "no" before they've done a ton of work +that now has to be thrown away or drastically re-written. + +## Step Two: Examine the main parts of the CL {#step_two} + +Find the file or files that are the "main" part of this CL. Often, there is one +file that has the largest number of logical changes, and it's the major piece of +the CL. Look at these major parts first. This helps give context to all of the +smaller parts of the CL, and generally accelerates doing the code review. If the +CL is too large for you to figure out which parts are the major parts, ask the +developer what you should look at first, or ask them to +[split up the CL into multiple CLs](../developer/small-cls.md). + +If you see some major design problems with this part of the CL, you should send +those comments immediately, even if you don't have time to review the rest of +the CL right now. In fact, reviewing the rest of the CL might be a waste of +time, because if the design problems are significant enough, a lot of the other +code under review is going to disappear and not matter anyway. + +There are two major reasons it's so important to send these major design +comments out immediately: + +- Developers often mail a CL and then immediately start new work based on that + CL while they wait for review. If there are major design problems in the CL + you're reviewing, they're also going to have to re-work their later CL. You + want to catch them before they've done too much extra work on top of the + problematic design. +- Major design changes take longer to do than small changes. Developers nearly + all have deadlines; in order to make those deadlines and still have quality + code in the codebase, the developer needs to start on any major re-work of + the CL as soon as possible. + +## Step Three: Look through the rest of the CL in an appropriate sequence {#step_three} + +Once you've confirmed there are no major design problems with the CL as a whole, +try to figure out a logical sequence to look through the files while also making +sure you don't miss reviewing any file. Usually after you've looked through the +major files, it's simplest to just go through each file in the order that +the code review tool presents them to you. Sometimes it's also helpful to read the tests +first before you read the main code, because then you have an idea of what the +change is supposed to be doing. + +Next: [Speed of Code Reviews](speed.md) diff --git a/review/reviewer/pushback.md b/review/reviewer/pushback.md new file mode 100644 index 0000000..3cd11c8 --- /dev/null +++ b/review/reviewer/pushback.md @@ -0,0 +1,83 @@ +# Handling pushback in code reviews + +[TOC] + +Sometimes a developer will push back on a code review. Either they will disagree +with your suggestion or they will complain that you are being too strict in +general. + +## Who is right? {#who_is_right} + +When a developer disagrees with your suggestion, first take a moment to consider +if they are correct. Often, they are closer to the code than you are, and so +they might really have a better insight about certain aspects of it. Does their +argument make sense? Does it make sense from a code health perspective? If so, +let them know that they are right and let the issue drop. + +However, developers are not always right. In this case the reviewer should +further explain why they believe that their suggestion is correct. A good +explanation demonstrates both an understanding of the developer's reply, and +additional information about why the change is being requested. + +In particular, when the reviewer believes their suggestion will improve code +health, they should continue to advocate for the change, if they believe the +resulting code quality improvement justifies the additional work requested. +**Improving code health tends to happen in small steps.** + +Sometimes it takes a few rounds of explaining a suggestion before it really +sinks in. Just make sure to always stay [polite](comments.md#courtesy) and let +the developer know that you *hear* what they're saying, you just don't *agree*. + +## Upsetting Developers {#upsetting_developers} + +Reviewers sometimes believe that the developer will be upset if the reviewer +insists on an improvement. Sometimes developers do become upset, but it is +usually brief and they become very thankful later that you helped them improve +the quality of their code. Usually, if you are [polite](comments.md#courtesy) in +your comments, developers actually don't become upset at all, and the worry is +just in the reviewer's mind. Upsets are usually more about +[the way comments are written](comments.md#courtesy) than about the reviewer's +insistence on code quality. + +## Cleaning It Up Later {#later} + +A common source of push back is that developers (understandably) want to get +things done. They don't want to go through another round of review just to get +this CL in. So they say they will clean something up in a later CL, and thus you +should LGTM *this* CL now. Some developers are very good about this, and will +immediately write a follow-up CL that fixes the issue. However, experience shows +that as more time passes after a developer writes the original CL, the less +likely this clean up is to happen. In fact, usually unless the developer does +the clean up *immediately* after the present CL, it never happens. This isn't +because developers are irresponsible, but because they have a lot of work to do +and the cleanup gets lost or forgotten in the press of other work. Thus, it is +usually best to insist that the developer clean up their CL *now*, before the +code is in the codebase and "done." Letting people "clean things up later" is a +common way for codebases to degenerate. + +If a CL introduces new complexity, it must be cleaned up before submission +unless it is an [emergency](../emergencies.md). If the CL exposes surrounding +problems and they can't be addressed right now, the developer should file a bug +for the cleanup and assign it to themselves so that it doesn't get lost. They +can optionally also write a TODO comment in the code that references the filed +bug. + +## General Complaints About Strictness {#strictness} + +If you previously had fairly lax code reviews and you switch to having strict +reviews, some developers will complain very loudly. Improving the +[speed](speed.md) of your code reviews usually causes these complaints to fade +away. + +Sometimes it can take months for these complaints to fade away, but eventually +developers tend to see the value of strict code reviews as they see what great +code they help generate. Sometimes the loudest protesters even become your +strongest supporters once something happens that causes them to really see the +value you're adding by being strict. + +## Resolving Conflicts {#conflicts} + +If you are following all of the above but you still encounter a conflict between +yourself and a developer that can't be resolved, see +[The Standard of Code Review](standard.md) for guidelines and principles that +can help resolve the conflict. diff --git a/review/reviewer/speed.md b/review/reviewer/speed.md new file mode 100644 index 0000000..47462b1 --- /dev/null +++ b/review/reviewer/speed.md @@ -0,0 +1,142 @@ +# Speed of Code Reviews + +[TOC] + +## Why Should Code Reviews Be Fast? {#why} + +**At Google, we optimize for the speed at which a team of developers can produce +a product together**, as opposed to optimizing for the speed at which an +individual developer can write code. The speed of individual development is +important, it's just not _as_ important as the velocity of the entire team. + +When code reviews are slow, several things happen: + +* **The velocity of the team as a whole is decreased.** Yes, the individual, + who doesn't respond quickly to the review, gets other work done. However, + new features and bug fixes for the rest of the team are delayed by days, + weeks, or months as each CL waits for review and re-review. +* **Developers start to protest the code review process.** If a reviewer only + responds every few days, but requests major changes to the CL each time, + that can be frustrating and difficult for developers. Often, this is + expressed as complaints about how "strict" the reviewer is being. If the + reviewer requests the _same_ substantial changes (changes which really do + improve code health) but responds _quickly_ every time the developer makes + an update, the complaints tend to disappear. **Most complaints about the + code review process are actually resolved by making the process faster.** +* **Code health can be impacted.** When reviews are slow, there is increased + pressure to allow developers to submit CLs that are not as good as they + could be. Slow reviews also discourage code cleanups, refactorings, and + further improvements to existing CLs. + +## How Fast Should Code Reviews Be? {#fast} + +If you are not in the middle of a focused task, **you should do a code review +shortly after it comes in.** + +**One business day is the maximum time it should take to respond** to a code +review request (i.e. first thing the next morning). + +Following these guidelines means that a typical CL should get multiple rounds of +review (if needed) within a single day. + +## Speed vs. Interruption {#interruption} + +There is one time where the consideration of personal velocity trumps team +velocity. **If you are in the middle of a focused task, such as writing code, +don't interrupt yourself to do a code review.** Research has shown that it can +take a long time for a developer to get back into a smooth flow of development +after being interrupted. So interrupting yourself while coding is actually +_more_ expensive to the team than making another developer wait a bit for a code +review. + +Instead, wait for a break point in your work before you respond to a request for +review. This could be when your current coding task is completed, after lunch, +returning from a meeting, coming back from the microkitchen, etc. + +## Fast Responses {#responses} + +When we talk about the speed of code reviews, it is the _response_ time that we +are concerned with, as opposed to how long it takes a CL to get through the +whole review and be submitted. The whole process should also be fast, ideally, +but **it's even more important for the _individual responses_ to come quickly +than it is for the whole process to happen rapidly.** + +Even if it sometimes takes a long time to get through the entire review +_process_, having quick responses from the reviewer throughout the process +significantly eases the frustration developers can feel with "slow" code +reviews. + +If you are too busy to do a full review on a CL when it comes in, you can still +send a quick response that lets the developer know when you will get to it, +suggest other reviewers who might be able to respond more quickly, or +[provide some initial broad comments](navigate.md). (Note: none of this means +you should interrupt coding even to send a response like this—send the +response at a reasonable break point in your work.) + +**It is important that reviewers spend enough time on review that they are +certain their "LGTM" means "this code meets [our standards](standard.md)."** +However, individual responses should still ideally be [fast](#fast). + +## Cross-Time-Zone Reviews {#tz} + +When dealing with time zone differences, try to get back to the author when they +are still in the office. If they have already gone home, then try to make sure +your review is done before they get back to the office the next day. + +## LGTM With Comments {#lgtm-with-comments} + +In order to speed up code reviews, there are certain situations in which a +reviewer should give LGTM/Approval even though they are also leaving unresolved +comments on the CL. This is done when either: + +* The reviewer is confident that the developer will appropriately address all + the reviewer's remaining comments. +* The remaining changes are minor and don't _have_ to be done by the + developer. + +The reviewer should specify which of these options they intend, if it is not +otherwise clear. + +LGTM With Comments is especially worth considering when the developer and +reviewer are in different time zones and otherwise the developer would be +waiting for a whole day just to get "LGTM, Approval." + +## Large CLs {#large} + +If somebody sends you a code review that is so large you're not sure when you +will be able to have time to review it, your typical response should be to ask +the developer to +[split the CL into several smaller CLs](../developer/small-cls.md) that build on +each other, instead of one huge CL that has to be reviewed all at once. This is +usually possible and very helpful to reviewers, even if it takes additional work +from the developer. + +If a CL *can't* be broken up into smaller CLs, and you don't have time to review +the entire thing quickly, then at least write some comments on the overall +design of the CL and send it back to the developer for improvement. One of your +goals as a reviewer should be to always unblock the developer or enable them to +take some sort of further action quickly, without sacrificing code health to do +so. + +## Code Review Improvements Over Time {#time} + +If you follow these guidelines and you are strict with your code reviews, you +should find that the entire code review process tends to go faster and faster +over time. Developers learn what is required for healthy code, and send you CLs +that are great from the start, requiring less and less review time. Reviewers +learn to respond quickly and not add unnecessary latency into the review +process. +But **don't compromise on +the [code review standards](standard.md) or quality for an imagined improvement +in velocity**—it's not actually going to make anything happen more +quickly, in the long run. + +## Emergencies + +There are also [emergencies](../emergencies.md) where CLs must pass through the +_whole_ review process very quickly, and where the quality guidelines would be +relaxed. However, please see [What Is An Emergency?](../emergencies.md#what) for +a description of which situations actually qualify as emergencies and which +don't. + +Next: [How to Write Code Review Comments](comments.md) diff --git a/review/reviewer/standard.md b/review/reviewer/standard.md new file mode 100644 index 0000000..4e2b826 --- /dev/null +++ b/review/reviewer/standard.md @@ -0,0 +1,112 @@ +# The Standard of Code Review + +[TOC] + +The 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. + +In order to accomplish this, a series of trade-offs have to be balanced. + +First, developers must be able to _make progress_ on their tasks. If you never +submit an improvement to the codebase, then the codebase never improves. Also, +if a reviewer makes it very difficult for _any_ change to go in, then developers +are disincentivized to make improvements in the future. + +On the other hand, it is the duty of the reviewer to make sure that each CL is +of such a quality that the overall code health of their codebase is not +decreasing as time goes on. This can be tricky, because often, codebases degrade +through small decreases in code health over time, especially when a team is +under significant time constraints and they feel that they have to take +shortcuts in order to accomplish their goals. + +Also, a reviewer has ownership and responsibility over the code they are +reviewing. They want to ensure that the codebase stays consistent, maintainable, +and all of the other things mentioned in +["What to look for in a code review."](looking-for.md) + +Thus, we get the following rule as the standard we expect in code reviews: + +**In general, 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.** + +That is _the_ senior principle among all of the code review guidelines. + +There are limitations to this, of course. For example, if a CL adds a feature +that the reviewer doesn't want in their system, then the reviewer can certainly +deny approval even if the code is well-designed. + +A key point here is that there is no such thing as "perfect" code—there is +only _better_ code. Reviewers should not require the author to polish every tiny +piece of a CL before granting approval. Rather, the reviewer should balance out +the need to make forward progress compared to the importance of the changes they +are suggesting. Instead of seeking perfection, what a reviewer should seek is +_continuous improvement_. A CL that, as a whole, improves the maintainability, +readability, and understandability of the system shouldn't be delayed for days +or weeks because it isn't "perfect." + +Reviewers should _always_ feel free to leave comments expressing that something +could be better, but if it's not very important, prefix it with something like +"Nit: " to let the author know that it's just a point of polish that they could +choose to ignore. + +Note: Nothing in this document justifies checking in CLs that definitely +_worsen_ the overall code health of the system. The only time you would do that +would be in an [emergency](../emergencies.md). + +## Mentoring + +Code review can have an important function of teaching developers something new +about a language, a framework, or general software design principles. It's +always fine to leave comments that help a developer learn something new. Sharing +knowledge is part of improving the code health of a system over time. Just keep +in mind that if your comment is purely educational, but not critical to meeting +the standards described in this document, prefix it with "Nit: " or otherwise +indicate that it's not mandatory for the author to resolve it in this CL. + +## Principles {#principles} + +* Technical facts and data overrule opinions and personal preferences. + +* On matters of style, the [style guide](http://google.github.io/styleguide/) + is the absolute authority. Any purely style point (whitespace, etc.) that is + not in the style guide is a matter of personal preference. The style should + be consistent with what is there. If there is no previous style, accept the + author's. + +* **Aspects of software design are almost never a pure style issue or just a + personal preference.** They are based on underlying principles and should be + weighed on those principles, not simply by personal opinion. Sometimes there + are a few valid options. 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 author. + Otherwise the choice is dictated by standard principles of software design. + +* 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. + +## Resolving Conflicts {#conflicts} + +In any conflict on a code review, the first step should always be for the +developer and reviewer to try to come to consensus, based on the contents of +this document and the other documents in [The CL Author's Guide](../developer/) +and this [Reviewer Guide](index.md). + +When coming to consensus becomes especially difficult, it can help to have a +face-to-face meeting or a VC between the reviewer and the author, instead of +just trying to resolve the conflict through code review comments. (If you do +this, though, make sure to record the results of the discussion in a comment on +the CL, for future readers.) + +If that doesn't resolve the situation, the most common way to resolve it would +be to escalate. Often the +escalation path is to a broader team discussion, having a TL weigh in, asking +for a decision from a maintainer of the code, or asking an Eng Manager to help +out. **Don't let a CL sit around because the author and the reviewer can't come +to an agreement.** + +Next: [What to look for in a code review](looking-for.md)