Code Reivew规范

177 阅读9分钟

本文的名词解释:

  • CR:code review
  • CL:change list(指这次改动)

前言

这篇文章是站在公司的角度基于 Google的代码审查指南 和笔者实际经验的个人总结,所以相对原文(Google的代码审查指南)有适当的删减,想看完整版的请阅读Google的代码审查指南

该规范的目的旨在提高公司内部项目的代码库质量。

原则

  1. 技术事实和数据应该凌驾于意见和个人偏好之上;在代码风格方面,风格指南是绝对权威,任何不在指南中的点(如空格等)都是个人偏好的问题,这些点原则上应该与现有的风格保持一致,但是如果这些点现在还没有形成一定的风格,那么应该接受作者的风格。

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

CR的标准

  1. Code Reivew的主要目的是确保代码库的 整体代码 运行状况随着 时间的推移得到改善,Code Review的所有工具和流程都为此而设计。

  2. 为了实现这个目标,必须 综合考虑许多因素,并且做出 取舍和平衡

  3. 审查者有责任确保每个CL都具有这样的质量:即随着时间的推移,代码库的整体代码健康状况不会降低。这可能会很棘手,特别是当团队的交付受到严重的时间限制并且他们觉得必须采取捷径才能实现目标时。

  4. 没有 完美 的代码,只有 更好的代码,审查者应该权衡发展的需要和别人的建议;不应该追求完美,而应追求持续改进,作为一个整体,如果CL能提高整个系统的可维护性、可读性、可理解性,那么不应该因为它不 “完美” 而拒绝。

  5. 如果开发人员有些地方做的很好,应该告诉并鼓励开发人员,但是如果这个改进不是很重要,可以告诉作者这种改进只是一种锦上添花的效果,作者可以选择忽略避免重复工作。

CR关注点

  1. 在CR中最重要的是看CL的整体设计与结构。CL中不同代码段的交互是否有意义?它与系统的其他部分是否集成良好?现在是添加此功能的合适时机吗?

  2. 审查者应该按照 风格指南开发规范 进行检查。

  3. 审查者应该考虑一些边缘状况,寻找并发性问题,尝试像用户一样思考,并确保不会看到仅通过阅读代码就能发现的错误。

  4. 审查者应该严查使用了设计模式的地方(例如使用了单例,那么就检查是否一定需要使用)。

  5. 如果有必要的话可以自己去验证CL。有些时候在阅读代码时,很难理解某些更改会对用户产生怎样的影响,例如UI更改。对于这样的更改,如果不方便自己测试,可以让开发人员演示该功能。

  6. 在CR期间另一个特别重要的事情是查看代码中是否存在某种 并发编程,理论上可能导致死锁或竞争条件,这些类型的问题很难通过运行代码来检测,通常需要有人(开发人员或审查者)仔细考虑它们以确保不会引入问题。

复杂度

  1. 代码的复杂度是否超过预期?有没有更简单的实现方式?单行代码是否过于复杂?函数或方法是否过于复杂?类是否过于复杂?“复杂” 通常意味着该代码很难阅读,也意味着 “当开发人员试图调用或修改这段代码时,他们很可能会引入BUG”。

  2. 有一种复杂性是过度设计造成的,开发者让那段代码过度通用化,超过了原本所需要的,或者是增加了系统目前不需要的功能;审查者应特别注意一下过度设计,鼓励开发者解决他们现在需要解决的问题,而不是推测将来可能需要解决的问题,当那些问题出现的时候再去解决它们,因为那个时候你可以更清晰的看见问题的样子。

如何浏览CL

  1. 用宏观的角度来看待改动,查看CL描述以及它做了什么。

  2. 找到CL最核心的部分文件,首先查看这些主要部分。这有助于为CL的其他较小部分提供逻辑,而且这样可以提高CR速度。如果CL太大导致无法确定哪里是主要部分时,请向开发者询问重要的部分,或者要求他们将CL拆分为多个CL。如果在主要部分发现存在主要的设计问题,应该立即告诉开发者修改。因为如果设计问题足够严重的话,继续CR其他部分的代码可能只是浪费时间。

浏览每一行代码

  1. 仔细审查每一行代码。有些东西,例如资料文件、生成的代码、大型数据结构,你可以稍微扫过,但是不要扫过开发者写的类、函数、方法、代码区块,更不能假设它内部是没问题的。

  2. 如果代码过于复杂并且需要减慢审查速度时,那么请你告诉开发者这件事,让他们为这段代码做出解释。你要求开发者去说清楚这段代码时,同时也在帮助未来的开发人员理解这些代码。

上下文

  1. 请结合上下文查看CL,该CL是否改善了整体系统的代码质量,会不会让整个系统更加复杂?是否缺少测试?千万不要授受会降低整体系统代码质量的CL。因为大多数系统是由于许多小改动的累积而变得复杂的。

测试

  1. 一般来说,测试应该添加到与生产代码相同的CL中,除非CL正在处理紧急情况。

  2. 审查者有必要确保测试是正确、合理、有用的。当代码真的有问题,测试是否会失败?如果被测试的程序发生改动时,测试是否会产生误报?每一个测试是否做出了简单而有用的断言?不同的测试方法之间是否适当分开?

  3. 请记住,测试代码也是需要维护的代码。不要因为测试不是主分支的一部分就接受测试中的复杂性。

解决冲突

  1. 在CR中遇到任何冲突,首先应该是开发人员和审查者通过本文档的内容和代码规范尝试达成共识。

  2. 当达成共识比较困难时,审查者可以和作者进行面对面沟通。如果还不能解决问题,可以让更多的人(例如项目管理者)参与进来一起讨论解决冲突。

紧急状况

  1. 紧急提交历史只能是一个小的更改:修复严重影响生产中用户的错误,处理紧迫的法律问题,关闭主要的安全漏洞等。

  2. 在紧急情况下,我们更关心整个代码审查过程的速度,仅在这种情况下,审查者应该更关心审查的速度和代码的正确性(它是否真的解决了紧急情况?)而不是其他任何事情。此外这种审核应该优先于所有其他代码审查。

  3. 但是,在紧急情况解决后,您应该再次查看紧急情况提交历史并对其进行更彻底的审查。

什么不是紧急情况?

  1. 以下情况不是紧急情况:
    • 想要在本周而不是下周发布(除非有一些实际的硬性发布期限,例如合同协议)。
    • 开发人员已经在一个功能上工作了很长时间,他们很想审查通过。
    • 今天是星期五,在开发人员开始周末之前审查通过会很棒。
    • ……

什么是硬期限?

  1. 硬期限指的是严格的截止日期,如果您错过它就 会发生灾难性事情 的截止日期。例如:

    • 为了履行合同义务,必须在某个日期之前完成。
    • 如果未在特定日期之前发布,您的产品将在市场上完全失败。
    • 一些硬件制造商每年只发货一次新硬件,如果您错过了向他们提交代码的截止日期,那可能是灾难性的。
    • ……
  2. 大多数截止日期都是软截止日期,而不是硬截止日期;它们代表了在某个时间完成某项功能的愿望,它们很重要,但不应该牺牲代码质量来达到目的。

CR太严格被抱怨怎么办?

  1. 提高CR的速度通常会让这些抱怨逐渐消失。可能需要数个月,但最终开发人员会看到严格的CR所带来的价值,因为严格的CR会帮助他们产生更优秀的代码。

  2. 如果你遵循这些准则,并且如果CR非常严格的话,后面你会发现整个CR流程会越来越快。因为开发者会知道什么是质量好的代码,并且在开始就提交一个很棒的CL。但不要为提高想象中的速度,而对CR标准和代码质量做出妥协,毕竟从长远来看它实际上并不会让任何事情发生的更快。

结语

  1. 可以转载,但是请注明来源。

参考

  1. Google的代码审查指南