Code Review 代码审查
关于 CR
代码审查(Code Review, CR)是项目进展到编码阶段非常重要的品质保证活动
目的
- CR 是一种用来确认方案设计和代码实现的质量保证机制，通过这个机制我们可以对代码、测试过程和注释进行检查
- CR 主要用来在软件工程过程中改进代码质量，通过 CR 可以达到如下目的
- 在项目早期发现代码中的 BUG
- 帮助初级开发人员学习高级开发人员的经验，达到知识共享
- 避免开发人员犯一些很常见，很普通的错误
- 保证项目组人员的良好沟通
- 项目或产品的代码更容易维护
前提
- 进入 CR 需要检查的条件如下
- CR 人员是否理解了 CR 的概念和 CR 将做什么
- 如果做 CR 的人员不能理解 CR 对项目成败和代码质量的重要程度，他们的做法可能就会是应付了事
- 代码是否已经正确的 build，build 使得代码已经不存在基本语法错误
- 我们总不希望高级开发人员或是主管将时间浪费在检查连编译都通不过的代码上吧
- 代码执行时功能是否正确
- CR 人员不负责检查代码的功能是否正确，需要复查的代码必须由开发人员或质量人员负责该代码的功能的正确性
- Review 人员是否理解代码
- 做复查的人员需要对该代码有一个基本的了解，其功能是什么，是哪一方面的代码，涉及到数据库或是通讯，这样才能采取针对性的检查
- 开发人员是否对代码做了单元测试
- 这一点也是为了保证 CR 前一些语法和功能问题已经得到解决，CR 人员可以将精力集中在代码的质量上
需要做什么
CR 主要检查代码中是否存在以下方面问题： 代码的一致性、编码风格、代码的安全问题、代码冗余、是否正确设计以满足需求(性能、功能）等等 以下内容参考了《Software Quality Assurance: Documentation and Reviews》一文中的代码检查部分
- 完整性检查(Completeness)
- 代码是否完全实现了设计文档中提出的功能需求
- 代码是否已按照设计文档进行了集成和 Debug
- 代码是否已创建了需要的数据库，包括正确的初始化数据
- 代码中是否存在任何没有定义或没有引用到的变量、常数或数据类型
- 一致性检查(Consistency)
- 代码的逻辑是否符合设计文档
- 代码中使用的格式、符号、结构等风格是否保持一致
- 正确性检查(Correctness)
- 代码是否符合制定的标准
- 所有的变量都被正确定义和使用
- 所有的注释都是准确的
- 所有的程序调用都使用了正确的参数个数
- 可修改性检查(Modifiability)
- 代码涉及到的常量是否易于修改(如使用配置、定义为类常量、使用专门的常量类等)
- 代码中是否包含了交叉说明或数据字典，以描述程序是如何对变量和常量进行访问的
- 代码是否只有一个出口和一个入口(严重的异常处理除外)
- 可预测性检查(Predictability)
- 代码所用的开发语言是否具有定义良好的语法和语义
- 是否代码避免了依赖于开发语言缺省提供的功能
- 代码是否无意中陷入了死循环
- 代码是否是否避免了无穷递归
- 健壮性检查(Robustness)
- 代码是否采取措施避免运行时错误(如数组边界溢出、被零除、值越界、堆栈溢出等)
- 结构性检查(Structuredness)
- 程序的每个功能是否都作为一个可辩识的代码块存在
- 循环是否只有一个入口
- 可追溯性检查(Traceability)
- 代码是否对每个程序进行了唯一标识
- 是否有一个交叉引用的框架可以用来在代码和开发文档之间相互对应
- 代码是否包括一个修订历史记录，记录中对代码的修改和原因都有记录
- 是否所有的安全功能都有标识
- 可理解性检查(Understandability)
- 注释是否足够清晰的描述每个子程序
- 是否使用到不明确或不必要的复杂代码，它们是否被清楚的注释
- 使用一些统一的格式化技巧(如缩进、空白等）用来增强代码的清晰度
- 是否在定义命名规则时采用了便于记忆，反映类型等方法
- 每个变量都定义了合法的取值范围
- 代码中的算法是否符合开发文档中描述的数学模型
- 可验证性检查(Verifiability)
- 代码中的实现技术是否便于测试
步骤
- 代码编写者和代码审核者坐在一起，由代码编写者按照设计文档中的用例依次讲解自己负责的代码和相关逻辑
- 可采用从前端到后台的方式，例如从 Web 层到 DAO 层
- 代码审核者在此过程中可以随时提出自己的疑问，同时积极发现隐藏的 bug
- 对这些 bug 记录在案
- 代码编写者修改后再次提交审核
- 代码审核者对 bug 记录进行回验
- 代码讲解完毕后，代码审核者给自己安排几个小时再对代码审核一遍
- 代码需要一行一行静下心看。同时代码又要全面的看，以确保代码整体上设计优良
- 代码审核者根据审核的结果编写“审核报告”
- “审核报告”中记录发现的问题及修改建议
- 将“审核记录”和“审核结果”提交至 git
- 代码编写者从 git 拉取，根据“审核报告”给出的修改意见，修改好代码，有不清楚的地方可积极向代码审核者提出
- 代码编写者 bug fix 完毕之后提交代码审核者再次进行审核
- 通过审核则更新审核结果并提交至 git
- 审核通过的代码不能再进行修改，需要修改必须重新进行审核流程
- 代码审核者把 CR 中发现的有价值的问题更新到”代码审核规范”的文档中，对于特别值得提醒的问题可群发 email 给所有技术人员
提示：CR 必备的文档
- “代码审核规范”文档：记录代码应该遵循的标准
- 代码审核者根据这些标准来 CR 代码，同时在 CR 过程中不断完善该文档
标准
代码审核的基础是 设计文档规范、代码规范、日志规范、测试代码规范。针对新增的业务场景和设计尚未有规范时应先确立规范后进行代码审核流程
执行
一个标准的 CR 活动应该分为三个阶段
事前准备阶段
在一次 CR 前，对以下内容进行充分准备
- CR 的对象
- 在准备 CR 代码对象时，我们要注意代码的数量，如果代码量比较大，要对代码进行必要的分解，确定其中的关键代码，对关键代码进行 CR，可以达到举一反三的目的
- CR 的内容
- 我们对代码的审查内容很多，如代码的编写是否规范（注释的书写格式、命名规范等）、技术处理规范（异常处理、日志处理、代码组织结构等）、业务实现等
- 我们不能希望通过一次 CR 活动，完成所有这些内容的审查，因此我们必须设定本次 CR 活动内容界限，确定审查重点
- 评审规范和标准
- 在 CR 前设计确定评审规范和标准是必要，通过规范和标准我们在审查过程中可以有据可依，有理可循，而且还可以做到标准统一
- 选择 CR 活动的参与者
- 在 CR 开始前，必须把本次 CR 活动的对象、审查内容以及审查的规范和标准通报给所有的参与者
- 选择 CR 活动的实施方式
- CR 活动有很多形式可供我们选择，我们可以根据实际情况选择桌面式 CR、演示讲解式 CR、一对一的座位 CR 等等
实施阶段
充分的事前准备，只是做好 CR 活动的前提，在 CR 实施过程中，我们要做好以下工作
- 准确记录
- 对于 CR 过程发现的问题，我们必须清晰准确的记录，可以使用问题点记录单，明确记录的项目和内容
- 讲解与提问
- CR 过程中，要采用代码作者讲解和审查者提问方式。审查者不能只在发现问题时提问，同时也要根据本次审查的内容要求代码作者对某个特定问题的讲解
- 逐项审查
- 对事前确定的审查内容，要逐项审查，不能因为时间不足等因素一扫而过
- 注意气氛
- 实施审查时，要营造一个讨论问题、解决问题的氛围，不能把审查会搞成批判会，这样会影响相关人员的积极性
事后跟踪
- 确认发现的问题
- CR 结束后，对发现的问题，首先需要确定以下内容
- 问题点的难易程度以及影响的范围
- 解决问题的责任者和问题点修正结果的确认者
- 解决问题点的时限
- 修正问题责任者
- 对于修正问题责任者，在问题点的修正过程中，要三方面内容的记录
- 问题点的原因
- 解决问题点的对策
- 修正的内容
- 修正结果确认者
- 做为修正结果的确认者，必须按照事前约定的时限及时的对修正结果进行全面的确认
注意事项
经常进行 CR
- 要 Review 的代码越多，那么要重构，重写的代码就会越多。而越不被程序作者接受的建议也会越多，唾沫口水战也会越多
- 建议每一个功能，每一个用例完成之后就进行审核
- 程序员代码写得时候越长，程序员就会在代码中加入越来越多的个人的东西
- 越接近软件发布的最终期限，代码也就不能改得太多
- 先 review 设计实现思路
- review 设计模式
- review 成形的骨干代码
- review完成的代码
- 如果程序复杂的话，需要拆成几个单元或模块分别 review
- 每次 review 的代码再 1000 行以内，时间不超过 1.5 小时
CR 不要太正式，而且要短
忘了那个代码评审的 Checklist 吧，走到你的同事座位跟前，像请师父一样请他坐到你的电脑面前，然后，花 5 分钟给他讲讲你的代码，给他另外一个 5 分钟让他给你的代码提提意见，这比什么都好。而如果你用了一个 Checklist，让这个事情表现得很正式的话，下面两件事中必有一件事会发生：
- 只有在 Checklist 上存在的东西才会被 Review
- CR 变成了一种礼节性的东西，你的同事会装做很关心你的代码，但其实他心里想着尽快地离开你
只有不正式的 CR 才会让你和评审者放轻松，人只有放松了，才会表现得很真实，很真诚。记住R eview 只不过是一种形式，而只有在相互信任中通过相互的讨论得到了有意义和有建设性的建议和意见，那才是最实在的。不然，作者和评审者的关系就会变成小偷和警察的关系。
尽可能的让不同的人 Reivew 你的代码
如果可能的话，不要总是只找一个人来 Review 你的代码，不同的人有不同的思考方式，有不同的见解，所以，不同的人可以全面的从各个方面评论你的代码。 但不要太多了，人多嘴杂反而适得其反，基本上来说，不要超过3个人，这是因为，这是一个可以围在一起讨论的最大人员尺寸。
下面是几个优点
- 从不同的方向（实现、需求、用户使用、算法、性能效率、易读性、扩展性）评审代码总是好的
- 会有更多的人帮你在日后维护你的代码
- 这也是一个增加团队凝聚力的方法
- 不要超过 3 个人，减少讨论的代价
保持积极的正面的态度
程序员最大的问题就是“自负”，尤其当我们 Reivew 别人的代码的时候，我已经见过无数的场面，程序员在 CR 的时候，开始抨击别人的代码，质疑别人的能力。太可笑了，我分析了一下，这类的程序员其实并没有什么本事，因为他们指责对方的目的是想告诉大家自己有多么的牛，靠这种手段来表现自己的程序员，其实是就是传说中所说的“半瓶水”。
所以，无论是代码作者，还是评审者，都需要一种积极向上的正面的态度，作者需要能够虚心接受别人的建议，因为别人的建议是为了让你做得更好；评审者也需要以一种积极的正面的态度向作者提意见，因为那是和你在一个战壕里的战友。记住，你不是一段代码，你是一个人！
学会享受 CR
这可能是最重要的一个提示了，如果你到了一个人人都喜欢 CR 的团队，那么你会进入到一个生机勃勃的地方，在那里，每个人都能写出质量非常好的代码，在那里，你不需要经理的管理，团队会自适应一切变化，他们相互学习，相互帮助，不仅仅是写出好的代码，而且团队和其中的每个人都会自动进化，最关键的是，这个是一个团队
操作
自我审查
- 提交代码前自我审查，添加对代码的说明
- 所有团队成员先进行自我审查，除了检查代码的正确性以外，还可以完成
- 对代码添加注释，说明本次修改背后的原因，方便其他人进行审查
- 修正编码风格，尤其是一些关键数据和方法的命名，提高代码的可读性
- 从全局审视设计，是否完整的考虑了所有情景
- 提交自己的单元测试报告
开发互审
任意两名开发人员（建议不要固定配对，避免思维定式）进行交叉代码审查
- 代码编写者：
- 准备所开发的代码相关的全部资料列表：需求、设计文档、代码工程、类、方法、配置文件、数据库修改等全部资料的版本号等详细信息
- 向代码审查者全面介绍代码的目标和设计实现
- 代码审查者
- 根据需求文档、设计文档、开发规范进行代码审查（业务、日志、测试）
- 将审查结果提交至 git
- 代码编写者对问题进行修改并由代码审查者复审，复审结果提交至 git 保留
- 代码审查者对审查的代码负责
上级审查
开发互审完成后，由上级进行上级审查，流程与开发互审相同，对于三次复审仍未通过的代码需要代码编写者进行组内检讨问题原因，并书面列出改进计划
冲突解决
当开发互审对于检查内容出现争议时由上级进行协调解决或逐级向上协调解决
附录 审核记录
- 审核记录如同修改记录一样，直接记录入代码头部，代码审核者修改审核记录后提交代码至 git 参考即可
- 之后的审核可以基于两次审核间的变更利用对比工具进行增量审核
示例如下
/** * 名称：xxxxx类 * 创建者：xx * 创建时间：2018-12-07 * 创建描述：实现xxxxxx * 修改者：xxx * 修改时间：2018-12-08 * 修改描述：添加xxx，修改xxx * 审核者：xxx * 审核时间：2018-12-08 * 审核描述：审核通过 * 审核者：xxx * 审核时间：2018-12-09 * 审核描述：审核不通过，xxxxxxxxxxxxx */
附录 审核结果
- 审核结果建议以表格的形式描述，每个问题分别列出
- 可通过标注行号来具体执行位置，给出合理的修改意见并说明标准
审核结果写入 commit message 中，以软表格的形式描述
docs(代码审核)：审核通过 docs(代码审核)：审核失败 1 日志不符合规范 问题：没有使用 log4j2，日志不规范 建议：建议使用 log4j2，包括包引用和代码修改 行号：53 行 2 命名不符合规范 问题：log 命名不符合规范 建议：修改为 logger 行号： 53 行