本文翻译自"Code Reviews"，来自于《97
 Things Every Programmer Should Know》一书中的某个章节。

你应该做代码评审。为什么呢？因为代码评审可以提高代码质量并且降低缺陷比例。但进行代码评审未必是因为你想到的那些理由。

由于之前有过一些代码评审的糟糕体验，因此许多程序员不喜欢代码评审。我曾经见过一些组织，它们要求所有代码在部署到生产环境之前必须通过一个正式的评审。多数情况下由架构师或一名主程序员进行这些评审，这种做法被戏称为“架构师评审一切”。这个要求被写在了他们的软件开发过程手册中，因此程序员必须遵守。可能有一些组织的确需要这样一个严格且正式的过程，不过大多数组织并不需要。在大多数组织中，这样做法只会适得其反。被评审者会感到他们就像是在等待一个假释裁决委员会的判决。而评审者既需要时间阅读源码，还需要时间跟上系统的最新进展，了解系统的全部细节。评审者很快就成为了这个过程的瓶颈。而这个过程也会很快地成为众矢之的并变得愈加糟糕。

代码评审的目的应该是共享知识和建立共同的编码指南，而不是简单地纠正代码中的错误。与其它程序员们分享你的代码使集体代码拥有权成为可能。让任意一个项目成员与组内其它人一起浏览代码。评审代码时，你应该尝试学习并理解这些代码，而不是去找错误。

代码评审过程中保持气氛和谐。确保意见是建设性的，而不是刻薄挖苦。为评审会议引入不同的评审角色，避免成员之间的年资影响代码评审。就角色举例来说，可以有一个专门关注文档的评审者，另外一个评审者重点关注异常，第三个人负责关注功能性。这种方法有助于在项目成员间分担评审负担。

每周进行一次例行的代码评审。在评审会议室里花上几个小时进行评审。每次会议按照一个简单循环的方式轮换被评审者。记住项目组组员承担的评审角色在每次会议上也要轮换。代码评审时带上新手。他们也许经验不足，不过他们从大学带来的新鲜知识可以提供一种不同的看法。带上专家，他们有经验和知识。他们可以更快更准确地识别出容易出错的代码。如果项目组拥有代码规范的检查工具的话，代码评审过程将更加容易和顺畅。那样的话，在代码评审会议上大家永远不会讨论代码格式问题。

让代码评审变得有趣也许才是代码评审成功的最为重要的因素。评审是关于人的评审。如果评审会议是痛苦且枯燥无味的，那么它很难激发出大家参与的热情。让它成为一种非正式的、以在项目组成员间共享知识为主要目的的代码评审吧。抛弃那些刻薄挖苦，用蛋糕或黄包餐（译注：全体成员一起参加的午餐）取而代之。

1. 提交以功能为单位
理由：

可以形成认知体系，熟悉该功能完整流程

2. 代码评审量

代码量控制在两千左右
或者讲解时长控制在 20分钟以内

理由：

过多的代码量评审，评审人员对功能体系容易疏忽
需要留给足够的时间给其他评审人员挑战，以及被评审人员的答辩

3. 提交模式

一次正式提交一次commit。多次提交合并commit

	git add		添加需要提交的文件名
	git stash -u -k		忽略其他文件，把现修改的隐藏起来，这样提交的时候就不会提交未被add的文件
	git commit -m "..."		哪里做了修改可写入
	git pull 		拉取合并
	git push 		推送到远程仓库
	git stash pop 		恢复之前忽略的文件
	git commit —amend  		合并commit


以个人分支merge request方式提交

4. 评审人员

必须有该项目组下，相关人员，两名参与。
其中一个人员为正式员工，或者在该项目下6个月以上经验

理由：

多人参与可以形成意见的挑战，讨论，趋于大同标准化
老员工参与，可以帮助发现隐藏的需要注意的事项

5. 评审时间

总体评审时间控制在30分钟左右。采取小步快跑
完成功能，自测完成，就可以提出评审。（禁止最后提测阶段才评审）

6. 评审方式

无条件，共享电脑屏幕
有条件，gerrit等相关工具

简介： 代码评审中同样存在着“Talk is cheap. Show me the code”，语言无力时，直接上代码吧。这就是我们今天要讨论的话题——代码评审中的代码协同。



作者 | 知忧

来源 | 阿里技术公众号

大神说：“Show me the code”，于是就有了代码评审。

“Talk is cheap. Show me the code.”

——Linus Torvalds, founder of Linux and Git.

代码评审中同样存在着“Talk is cheap. Show me the code”，语言无力时，直接上代码吧。这就是我们今天要讨论的话题——代码评审中的代码协同。

一 基于邮件列表的代码评审

这是一种和代码仓库松耦合的代码评审模式，100%的代码都要经由一位或多位“仁慈的独裁者”（benevolent dictator）代码评审后才能合并入代码仓库。这种开发模式还需要开发者掌握一些命令行操作技巧以便完成代码在仓库和邮件列表之间的转换。采用这个模式的项目不多，不过 Linux、Git 开源社区就是按照这种模式运作的。

1 代码和邮件的相互转换

代码转换为电子邮件，要使用 git format-patch 命令。例如下面的命令将指定范围的代码提交（例如在 origin/master 之后的新提交）转换为电子邮件：

git format-patch origin/master..HEAD

生成的补丁文件的格式如下所示：



邮件头中的 Subject: 字段是邮件标题，使用 [PATCH] 作为标题前缀，以提交说明的第一行作为标题内容。
	
更多的提交说明作为邮件内容，和邮件头之间用一个空行分隔开。
	
用分隔符 --- 作为提交说明的结束。
	
在分隔符 --- 和 diff --git 开始的补丁内容之间的文字被忽略。通常此处内容是提交的变更统计，开发者也可以在此处写入不宜列入提交说明中的附加说明。
git format-patch 命令有很多参数，要结合不同场景使用，例如：

一个特性由多个提交构成，分散在多个提交中的提交说明难以描述整个特性，可以使用 --cover-letter 参数，生成一封编号为 0000 的邮件，作为后续提交的摘要说明，便于评审者理解代码。
	
一个特性通常会多次迭代，就需要为每次迭代设置不同的版本。这就要用到 -v {num} 参数指定补丁的版本。版本将体现在邮件标题中，例如第二版本的补丁，邮件标题将使用 [PATCH v2] 作为前缀。
	
回复特定邮件，以便形成可追踪的邮件线索，使用参数 --in-reply-to="{Message-ID}"，为电子邮件生成相关的 In-Reply-To: 和 References: 头信息。
	
默认提交本身的作者、提交说明的签名区（trailer）提及的贡献者会自动添加为邮件的收件人。要添加更多参与者，可以使用 --to={email}、--cc={email} 参数。
将电子邮件转换为代码，则使用命令 git am [options...] mail... 。该命令会将邮件正确转换为 Git 仓库中的提交。

使用 git send-email 命令，将包含代码提交的邮件发送到邮件列表。

2 评审中的代码片段转换为提交

代码评审以邮件回复的方式完成。注意邮件回复都要求用纯文本格式，否则会被邮件服务器退信。

代码评审中发现小的文字错误，例如将 warning 写成了 waring，评审者可能做出如下简洁的回复：

s/waring/warning/

这种约定俗成的格式大概是源于 sed 命令实现文本替换的语法。

评审者有时候会在回复中贴上大段的代码补丁，为了使代码补丁和邮件上下文做出区分，会使用特殊的剪刀分隔符将邮件中的评论和代码补丁分隔开。

Subject: Re: whatever thread you're in

Somebody else said:
> blah blah blah

I disagree. You should do it like this instead:

-- >8 --
first line of commit message

more commit message
---

diff --git ...

上面是 Peff（Jeff King）在邮件中给出的一个示范，看到其中的剪刀分隔符了么？剪刀分隔符由多个减号（穿孔的分割线）和一个剪刀符号组成至少8个字符的分隔符。可选的分隔符有：-- 8< -- 、-- >8 -- 、-- %< -- 或 --- >% --- 等。

使用 git am --scissors 命令，能够识别邮件中的剪刀分隔符，将邮件中的代码转换为提交。

3 为提交贡献者署名

Git的提交元信息中只包含两个署名信息，一个是提交的原始作者（Author），一个是将提交合入仓库或者对提交做了修补的提交者（Committer），而在提交评审过程中有过贡献的人往往不只两人，如何致敬贡献者呢？Git 社区的实践是在提交尾部（trailer）添加贡献者签名。贡献者签名由一个被动语态的关键字和贡献者ID组成，例如：

Signed-off-by: User < Email > ：通常由代码的贡献者（Author）和代码合入时的提交者（Committer）提供的签名。可由命令 git commit -s 、 git am -s 等命令自动添加。
	
Reported-by: User < Email > ：问题的报告者。
	
Helped-by: User < Email >：对提交有过帮助的人。
	
Reviewed-by: User < Email > ：评审者。
可以通过 Git 项目仓库的提交历史，看到更多的签名示例。

4 使用 GitHub PR 实现代码到邮件的转换

一个名为 GitGitGadget 工具借助 GitHub 强大的扩展能力，通过向 gitgitgadget/git 仓库发送 pull request，实现提交到邮件的转换，并发送到 Git 项目的邮件列表中。使用 GitGitGadget 参与 Git 社区代码贡献详见。

二 GitHub 代码评审中的协同

GitHub 使用 pull request 进行代码评审，评论中的代码块儿也可以转换为提交。

1 代码评论中嵌入代码块

下图中，点击评论工具栏第一个按钮，可以在评论中嵌入代码块：



2 评论中代码块转换为提交

对 pull request 的源仓库具有写权限的用户，可以将评审中的代码库转换为提交，如下图所示：



于是代码评审中会增加一个新的修正提交。

GitHub 的这个功能对于代码评审中发现的一些小问题，还是非常方便的。但是大的修改就无能为力了。

3 线下评审

对于功能复杂的 pull request，在线上浏览代码不方便，也不能线上调试代码，这时线下获取并浏览代码，就非常有必要了。

GitHub 的代码仓库中为每一个代码评审设置了特殊的关联引用：

refs/pull/{ID}/head ：关联 pull request 的源提交。
	
refs/pull/{ID}/merge ：对于没有冲突的 pull request，这个引用指向一个成功的合并提交。
代码评审者使用如下命令可以获取 pull request （例如编号为 123 的 PR）指向的提交：

git fetch origin refs/pull/123/head
git switch -d FETCH_HEAD

评审者可以线下调试 pull request 指向的代码，但是对代码做出的本地修改，没有办法直接更新到线上的代码评审中。

阿里巴巴的云效Codeup，支持线下到线上的代码协同。

三 云效Codeup代码评审中的协同

无论是 GitHub 还是 Gitlab，开发者创建代码评审首先需要将代码推送到线上独立的分支中（无论是在线上的派生仓库，还是目标仓库），然后再通过网页选择来源仓库、分支及目标仓库、分支，创建代码评审。

GitHub 和 Gitlab 这种代码评审方式，或者要引入冗余的派生仓库，或者需要为开发者赋予在仓库中的写入权限，并容易引发杂乱的分支管理。

1 适合主干开发的无分支创建代码评审

云效 Codeup 可以通过 git push 命令在客户端直接创建代码评审，无需创建派生仓库或者在仓库中创建特性分支。例如在客户端执行如下命令：

git push origin HEAD:refs/for/master/topic1

该命令会在服务端创建新的代码评审，或者如果已经存在相同用户、相同命令创建的代码评审则会更新评审中的提交。

建议安装我们开源的 git-repo 工具，则可以用更简单的命令行，实现从客户端创建/更新代码评审。

git pr

2 线下评审，线上协同

和 GitHub 类似，云效 Codeup 创建的代码评审都有一个特殊引用相关联，格式为：refs/merge-requests/{ID}/head。

代码评审者可以使用 git fetch 命令获取特定的代码评审（以编号123为例）指向的代码，进行线下代码评审。

git fetch origin refs/merge-requests/123/head
git switch -d FETCH_HEAD

如果安装了 git-repo，可以使用下面更为简洁的命令：

git download 123

代码评审者除了可以在本地仓库中浏览、调试代码，还可以更新代码、创建提交，然后将本地新增提交更新到线上的代码评审中。命令示例如下：

git pr -c 123



在云效 Codeup，开发者和评审者可以基于代码评审进行更为流畅的代码协同。

3 Git proc-receive 挂钩

上述“线下评审、线上协同”功能的核心是 Git 的 proc-receive 挂钩和 report-status-v2 新能力。这一功能由阿里巴巴贡献给 Git 社区，并在 Git 2.29.0 发布。

云效 Codeup 汇集了阿里巴巴最新的代码托管、代码协同技术，希望能够造福更多中国和世界的开发者。

原文链接

本文为阿里云原创内容，未经允许不得转载。

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

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