原文链接: The CL author’s guide to getting through code review
CL作者指南-如何通过Code Review
名词解释
CR: 代码审阅(Code Review) CL: 谷歌软件开发的基本单位(Change List) Reviewer: 审阅提交代码的人 Developer: 提交CL的开发者 Author: 本文的Author指的是写CL的开发者 CL描述(description): 针对本次change list的描述
写好CL描述
译者注: 我理解, 在Google开发体系中的一个CL类似于我们的一个Commit提交。CL描述也就是Commit提交的信息。这样在下文理解CL描述会更顺畅一些。
CL描述是一个公开的内容, 它记录了本次CL中有哪些变更以及为什么要这样处理。这些内容将会成为永久地保留在我们的版本控制历史库中。并且, 在借来的时间里, 除了Reviewer可能会有其他很多人也阅读这份CL描述。
以后的开发者会根据CL描述的内容去搜索你提交的CL。可能会有人在没有详细文档的情况下, 根据一点点微弱的记忆或相关信息去寻找你提交的变更。如果所有重要的信息都写在代码中, 而不在CL描述中的话, 他们在定位你提交的代码时将会非常困难。
第一行内容
- 简短总结做了什么
- 它应该是完整的句子, 像一条命令(译者注: 就是祈使句==)
- 紧跟一个空行
CL描述的第一行应该是一个简洁的总结内容。它需要明确地概括本次CL做了什么事情, 然后附上一行空白行。后面的开发维护同学在浏览一段代码的历史版本历史库时, 他们将会看到的CL的第一行内容。所以CL描述的第一行内容应该具有有充足的信息, 可以让他们在不用阅读提交代码或者整段描述的情况下, 就能大概的了解这个CL做了什么。
传统来讲，CL描述的第一行内容应该是一个完整的句子。并且应该写成命令式语句(祈使句)。举个例子, "删除FizzBuzz的ROC并且用新系统替代它(Delete the FizzBuzz RPC and replace it with the new system)" 而不是"正在删除FizzBuzzRPC并用新系统替代它(Deleteing the FizzBuzz RPC and replacing it with the new system)". 不过, 不用将其余的描述也写成命令式语句。
译者注: 中文应该没有这个问题吧. 我觉得大家应该不会写"正在XXXX"
CL描述的主体应包含有效的信息
其余的描述部分(Body)需要包含足够的信息。它可以包括被解决的问题的简洁描述以及为什么这么处理是最好。如果这个技术方案有缺点的话，这些缺点也应该被记录下来。如果有相关的内容，也需要补全背景信息, 例如bug的代码行数, 压测的结果和设计文档的链接等。
即使是再小的CL也需要关注到细节。要将CL放在上下文中。
译者注: 再小的CL变更也要补充充分的上下文信息。
不好的CL描述
"Fix bug" 就是一个内容不充分的CL描述。什么BUG？你做了什么去解决它? 其他类似的不好的描述也包括了:
- Fix build
- Add patch
- Moving code from A to B
- Phase 1.
- Add convenience functions
- kill weird URLs
这里有一些是真实被提交的CL描述。他们的作者可能觉得他们已经提供了有用的信息, 但是这些内容并没有满足CL描述的要求。
好的CL描述
这里有一些比较好的CL描述的例子
功能变更
例子:
rpc: 移除RPC服务器消息列表的大小限制
类似于FizzBuzz的服务有非常大的信息并且重新使用是有好处的。使freelist更大, 并且添加了一个goroutnie, 可以随着时间缓慢释放空闲的列表。这样空闲的服务器最终会释放掉全部的列表入口。
CL描述的第一行内容很简洁地描述CL做了什么。其余的CL描述讲了被解决的问题, 为什么这是好的解决方案以及更多关于详细扩展的信息。
重构
例子:
Construct a Task with add TimeKeeper to use its TimeStr and Now methods. 使用
TimerKeeper重构了Task, 可以使用TimerStr和Now方法Task添加了一个
Now方法, 所以borglert的getter方法可以被移除(这个只被被OOMCandidate用来调用borglet的Now方法)。 这个替代了Borglet委托给TimeKeeper的方法。允许
Tasks支持Now方法是为了排除Borglet的依赖。最终, 从Task获取Now的协作者就可以去直接去用TimerKeeper, 但是这也只是为了当下的重构继续重构Borglet的层次结构将是长期目标
第一行内容描述了CL做了什么以及与过去相比d当前的变更是如何实现的。其余部分讲了详细的扩展, CL的上下文, 解决方案不理想的地方, 未来要做的事情及处理方向。它也解释了为什么要做这次CL变更。
需要一些上下文的小CL
例子:
对 status.py 创建了一个Python3的构建规则
这个让已经通过Python3使用它的用户去以来一个规则, 是原来构建规则的附属规则，替代来他们本地代码树的某个位置。它鼓励新的用户使用Python3如果他们可以的话，而不是使用Python2， 并且很有意义的简化了一些自动构建问题重构工具(现在正在被使用的)
第一句内容描述了CL实际上做了什么。 其余部分解释了这个变更的原因并给出了许多上下文。
在提交CL前, Review自己的描述内容
在Review时, CL会融合进有意义的变更。所以,在提交CL前. reviewCL描述 是很有价值的事情。这会保证CLK描述仍然记录CL所做的事情。
小的CL变更
为什么写小CL变更?
小而简洁的变更:
- 可以更快地被review。对于reviewer来说, 相比于抽出30分钟的时间review较大的CL变更, 花费几个5分钟去review小的CL是更容易的。
- 可以完整地被review。 对于较大的CL变更, reviewer和CL作者会被大量l来回移动的评论阻塞着, 甚者有时一些重要的地方可能会被错过或遗漏。
- 减少引入bug的可能性。因为做了比较小的CL变更, 对于你和你的reviewer推断变更所带来的影响以及审查是否有Bug引入是更容易的。
- 减少可能会因为被拒绝而浪费的工作。 如果你写了一个巨大的变更然而你的reviewer认为整个方向是错误的，那这工作就十分浪费了。
- 更容易去合并。 处理一个大的CL变更需要很长时间。 在这段时间里,也许会有其他提交。在你合并的时候, 将会遇到许多冲突，并且你仍需要经常去合并代码。
- 更容易去设计。 相比于一个大的CL变更的全部细节中提炼设计, 展示一个小的变更的设计和代码健壮性是非常容易的。
- 减少review的阻塞。 提交了一部分独立的变更, 当你等待的Review中的CL时, 这可以让你继续写代码。
- 更容易回滚。 一个大的提交将会更可能触及不少已经更新的文件在最初的CL提交与一个回滚的CL提交，这会让回滚变得复杂(中间的提交可能也需要回滚)。
记住, Reviewer们可以就因为一个单独的原因-"CL提交太大了",决定去拒绝你的变更提交。通常Reviewer们会感谢你杰出的贡献但是会要求你无论如何也要把很大的CL变更拆分成一系列小的变更。在你写完变更之后, 分割一个变更会消耗巨大的工作量。否则你会需要更多的时间去讨论为什么Reviewer应该接受你提交的大CL变更。所以写小的变更是更优先的
那什么是小?
通常来讲, 一个CL变更比较合适的大小是这个CL变更是一个完整独立的改动。那这意味着:
- CL只做了一个最小的变更, 这个变更只处理了一件事情。这通常是一个特性的一部分, 而不是完整的特性。 通常, 我们宁可接受的错误是CL变更太小了也不想有一个特别大的变更。 你需要和你的reviewer一起去制定一个可以接受的合适的CL大小。
- Reviewer需要理解的CL的东西(除了未来的变更)都应该在CL, CL描述, 已经存在的代码或他们已经review过的CL中。
- 当变更被合入的时候, 对于其他人来讲, 系统将会保持继续工作。
- 如果CL不是很小的话, 那这意味理解它的含义是很困难的。如果你添加了一个新API，那在相同的变更中, 也应该包括了API的用例, 这样reviewer能更好的理解API是如何被使用的。同时, 这也阻止引入了没有用的API。
那什么时候大的CL变更是OK的
这里有一些场景, 也许大的CL变更也不是特别糟糕:
- 通常可以把删除一个文件当作是一行代码变更，因为review时这不会花费特别多的时间。
- 有时一个大的CL变更是被一个自动重构工具生成的，并且Review的工作也仅仅是正常的检查和确认想要这个变更。这些CL可以大一些, 即使会有上面的一些警告。
通过文件分割
另一种分割CL变更的方式是对文件分组。这些文件需要根据不同的reviewer去审阅代码, 但这些又都是独立的整体的CL变更。
译者注: 应该想说的是, 一个变更涉及了多个文件。 举个例子: 你提交了修改了pb文件的变更和另一个CL变更关于使用proto文件的代码变更。在提交代码变更之前, 你需要提交protobuffer文件变更，但是他们能同时被review。 如果你这么做的话，在Review代码变更时, 你可能需要通知reviewer关于你写的另外一个PB文件变更提交，这样他们才会有你提交变更的背景信息。
另一个例子, 你提交了一个变更，它是一个代码变更和使用了这份代码的配置或者实践。这也是更容易回滚的, 因为配置/实践文件有时会比代码更快地被推送到产品上。
分离重构提交
通常, 在一个独立变革提交里去做重构是最好的, 尽量不要在feature变更或bug fixes做重构。
举个例子, 移动和重命名一个类应该在不同的变更中里。而不是在那个类的BUG fix的变更提交中。这种分离的变更提交, 对于reviewe来说是更容易去去理解每个变更带来的改变的。
小的清理, 例如修复一个本地变量名称可以被包含在一个feature或bugfix的变更中。当然这是根据开发者和reviewer的判断去决定, 在当前变更中, 包含一个重构代码是否让review更加困难。
让相关的测试代码在相同的变更提交中
避免分离测试代码到独立的变更中。测试验证你的代码应该在相同的CL变更中，即使这样会增加代码的行数。
然而，独立的测试修改首先也应该在分离的变更中，类似于refactorings guidelines。 这包括:
- 使用新测试验证提前存在的或已经提交的代码
- 重构测试代码(举个例子: 引入帮助函数)
- 引入更大的测试框架代码(例如整合测试)。
不要破坏构建
如果你有几个互相依赖的CL变更, 那么就要确保在每个CL提交后, 整个系统仍继续工作、。否则, 在你的CL提交中的几分钟(或者会更长如果有一些发生了意外的错误在你的最新CL提交中), 这可能会影响到其他人的构建。
无法让变更足够小
有时你将会遇到一些情况, 它们会让你的CL变更变得很大。当然这个是很罕见的。 不断练习写小CL的开发者几乎总是能寻找一个方式去将功能解构到一系列小的CL变更中。
在写一个大的CL前, 考虑是否可以先通过一个重构CL, 之后就可以有一个更好的方式去做一个更干净的扩展。还可以和你的团队一起讨论, 是否有人有更好想法关于如何通过小CL去扩展功能。
如果所有这些方案都不可以(那应该是真的很罕见了), 那需要提前知会你的Reviewer, 这将会有一个十分大的CL变更 在这种场景下, 变更通过review的流程会花费很长时间, 这里就要特别警惕不要引入bugs, 并且要十分谨慎的写完整测试用例。
如何处理Reviewer的评论
当你提交了CL变更去review后, Reviewer在你的CL上做一些评论。这里会有一些有用的事情去了解关于处理reviewer的评论。
不要把评论上升到个人
Review的目标是去维护我们代码和产品的质量。当Reviewer对你的代码评论了一个批评言论, 那应该去想的是他们在尝试帮助你, 代码以及公司, 而不是一个攻击你或你的能力的私人行为。
有时, Reviewers会将恼怒的情绪发泄在他们的评论中。 虽然对于Reviewer来说这不是一个好的实践, 但是作为一名开发者, 你应该准备好面对这些。一般这个时候, 要自己问自己, Reviewer是否正在尝试给到有建设性的事情？ 然后去做他们实际上表达的要改善的内容。
不要生气地回复Review的评论。这是严重违背职业道德的行为, 并这将会永远留在代码review工具中。如果你特别生气, 不能和善地回复评论, 那可以离开电脑一会或去做些其他的事情直到你感觉足够平静去友善的回复。
通常来讲, 如果Reviewer没有反馈一个充满建设性和友善的建议, 那可以向他们解释这种情况。如果你不能直接联系到Reviewer的话, 那就发送一封私人邮件。 友善地向他们解释你不喜欢的和你希望他们改善的地方。 如果他们仍然以不好的态度回复你, 或者它没有任何作用的话, 那么就可以上升到合适的管理者。
修复代码
如果Reviewer说他们有点看不懂你的代码, 那你首先应该是去整理代码本身。如果代码不能整理清晰, 就需要添加代码注释去解释为什么代码是这样的。 如果评论看起来毫无重点, 那你应该是在代码Review工具上去做一个解释。
如果Review不懂你的代码片段, 那其他读者可能也不会懂。在代码Review工具上写一个回复可能不会对以后的开发者有帮互助, 那就整理你的代码并添加注释确定可以帮助他们。
自己的思考
写一个CL会花费很多工作。通常, 最终提交去Review时你会感觉十分满意， 就像是它已经完成地足够好而且非常确定不会有更多的事情要做了，所以当Reviewer反馈评论这里有能被改进的事情时, 就会很容易去认为"这个评论是错的，Reviewer没有必要阻塞你,应该就是让你提交CL"。 然而无论你是多么的确定你的CL没有问题, 那也要花一点时间回去看看并且考虑是否Reviewer提供了有价值的反馈, 这将会帮助代码和公司本身。所以,第一反应应该是, "Reviewer是正确的吗?"
如果你不能回答那个问题, 那么reviewer可能需要重新整理他们的评论。
如果你已经思考过并且仍然认为你是对的, 那就需要回复并解释, 对现在的代码/用户/公司, 为什么你做的事情的方法是更好的。通常来说, Reviewers实际上只提供了建议并且他们想要的是你自己去思考什么是最好的。实际上,你知道更多关于用户, 代码或者Reviewer不知道的CL变更。那么, 把他们补充进去, 给Reviewer更多的上下文背景。 基于技术事实, 你可以和你的reviewer获得一些一致的意见。
解决冲突
解决冲突的第一步应该是和Code Reviewer获得一致意见。如果不能达成一致, 那么看下代码Review的标准, 在这种情况下, 它给了一些可以遵循的原则。
