【Google 代码评审之道】The Standard of Code Review （代码评审标准）

代码审查的主要目的是确保Google代码库的整体代码的健康改善。代码评审的所有工具和过程都是为此目的而设计的。

为了实现这一目标，必须平衡一系列的权衡。

首先，开发人员必须能够在他们的任务上取得进展。如果你从来没有提交改善代码库,那么代码永远不会提高。此外，如果审查人员使任何更改都很难进行，那么开发人员就没有动力在将来进行改进。

另一方面，审查员的职责是确保每个CL的质量，使其代码库的总体代码健康度不会随着时间的推移而下降。这可能很棘手，因为随着时间的推移，代码库常常会随着代码健康状况的小幅下降而退化，尤其是当团队面临重大的时间限制，并且他们觉得必须走捷径才能实现目标时。

此外，审查员对他们所审查的代码拥有所有权和责任。他们希望确保代码库保持一致、可维护，以及“在代码评审中寻找什么”中提到的所有其他内容。

因此，我们得到以下规则作为我们在代码评审中期望的标准:

一般来说，评审员应该赞成在CL处于一种状态时批准它，在这种状态下，即使CL不是完美的，它也肯定会改善正在处理的系统的整体代码健康状况。

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.

这是所有代码评审指南中的高级原则。

当然，这也有局限性。例如，如果CL添加了审查员不希望在系统中使用的特性，那么即使代码设计良好，审查员也可以拒绝批准。

这里的一个关键点是，没有“完美”的代码，只有更好的代码。审稿人不应该要求作者在批准之前对CL的每一个微小部分进行打磨。她认为，与他们所建议的改变的重要性相比，审稿人应该在取得进步的必要性上找到一个平衡点。审稿人不应该追求完美，而应该追求持续的改进。作为一个整体，这提高了系统的可维护性、可读性和可理解性，不应该因为系统不“完美”而推迟几天或几周。

评审人员应该随时留下评论，表示有些东西可以做得更好，但是如果不是很重要，可以在前面加上“Nit:”这样的前缀，让作者知道这只是一种修饰，他们可以选择忽略。

注意:本文档中没有任何内容证明签入CLs肯定会恶化系统的整体代码健康状况。只有在紧急情况下你才会这么做。

代码评审具有重要的功能，可以教开发人员关于语言、框架或一般软件设计原则的新知识。留下有助于开发人员学习新东西的注释总是好的。知识共享是提高代码的健康系统的一部分。请记住，如果您的评论纯粹是教育性的，但对于满足本文档中描述的标准并不是至关重要的，那么在它前面加上“Nit:”，或者以其他方式表明作者并不强制在本CL中解决它。

事实和数据凌驾于观点和个人偏好之上。

在风格问题上，风格指南是绝对权威的。任何不在样式指南中的纯样式点(空格等)都是个人偏好的问题。风格应该与现有的一致。如果没有之前的风格,接受作者的。

软件设计方面,几乎没有一个纯粹的风格问题或只是个人偏好。它们是建立在基本原则的基础上的，应该在这些原则的基础上加以衡量，而不仅仅是个人意见。有时有一些有效的选择。如果作者能够证明(通过数据或基于可靠的工程原理)几种方法是同样有效的，那么审稿人应该接受作者的偏好。否则，选择取决于软件设计的标准原则。

如果没有其他规则适用，那么审查员可能会要求作者与当前代码库中的内容保持一致，只要这不会恶化系统的整体代码健康状况。

对于代码评审的任何冲突，第一步都应该是开发人员和评审人员根据本文档和CL作者指南和本评审指南中的其他文档的内容，努力达成共识。

达成共识变得特别困难，在审稿人和作者之间进行一次面对面的会议或VC可能会有所帮助，而不仅仅是试图通过代码评审注释来解决冲突。(不过，如果您这样做了，请确保将讨论结果记录在CL的评论中，以供将来的读者参考。)

这并不能解决问题，最常见的解决方法是逐步升级。升级路径通常是更广泛的团队讨论，让TL参与进来，请求代码维护人员作出决策，或者请求Eng经理提供帮助。不要因为作者和审稿人不能达成一致意见而让CL无所事事。

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."

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.

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.

Technical facts and data overrule opinions and personal preferences.

On matters of style, the style guide 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.

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 and this Reviewer Guide.

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

原文：https://github.com/google/eng-practices/blob/master/review/reviewer/standard.md

google 代码评审