【Google 代碼評審之道】The Standard of Code Review （代碼評審标準）

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 Standard of Code Review

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

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​​ 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​​​ 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​​

​