Back to Software Engineering

See Also Script for Counting Code LinesGerrit

代码评审

1. Code Review Benefits

很多团队都有这个问题:代码本来开始设计得好好的,一段时间以后,代码就会变得难以理解,难以维护,难以修改。为什么?

最快腐烂的代码,一定有很多人在修改;相反,长期由一个人来维护的代码,就不会那么容易腐烂。因为一个人发生思路冲突的概率要低得多,代码更容易保持一致性;人多思路必然不完全一致,只有靠长期坚持不懈的沟通交流来统一思想。

所以,代码腐烂本质上是沟通问题。改进的办法就是加强沟通。而代码审查是一种很好的社交活动。

1.1. Four eyes catch more bugs

Catch bugs early to save hours of debugging

引入代码评审会缩短测试周期,并降低最终代码缺陷率(Bugs/100Lines)

1.2. Enforce coding standards

Keep overall readability & code quality high

提升代码可读性并提高代码质量--两个人都明白要好于只有一个人(一时)明白;

如果coder编码的时候就很清楚,只有评审通过的代码才能合入主干,那么写出的程序会有所不同:会变得更加整洁,有着较好的注释,结构也组织的不错。如果没有这样的机制,coder知道代码会在最后才会审查。因为不是马上就要检查,所以对他而言并不紧迫,因而不会想着先自检一遍。

1.3. Mentoring of new developers

Learn from mistakes without breaking stuff

帮助新人学习并养成好习惯,对新人最好的监督方法就是代码评审

1.4. Establish trust relationships

Prepare for more delegation

代码评审促进知识共享,增强人力复用。

在很多的开发团队中,每个人都会负责并且专注于一个核心模块。除非别的同事负责的模块出现问题导致自己的代码不能运行,否则他们是不会去关注别人的工作。这样产生的结果是,每一个模块的代码只有一个人比较熟悉。假如事不凑巧,那位程序员正好休假或者离开了公司,那么没有人了解那些代码了。如果有代码审查的环节,那么至少会有两个人熟悉代码——代码的作者和审阅者。审阅者虽然没有作者对代码那么了解——但是他同样熟悉代码的设计和结构。

1.5. Good alternative to pair programming

asynchronous and across locations

另外一种结对编程,不受场地限制

1.6. 提升团队的设计开发能力

代码审查能帮助团队成员从彼此的错误中汲取经验,并成为更好的程序员。通过简单的反馈意见,公司可以提高了其开发员的水平。开发员重视审查,因为他们知道这将帮助他们成长。

当代码审查以小组为单位进行时,整个团队都得以提高。但更好的是,代码质量也得到提升,并易于维护。

1.7. 激发团队凝聚力

人们认为代码审查仅仅是寻找漏洞,但它却能把人团结在一起,它可以提供的远远超过你所预期的。

有很多这样的例子在执行代码审查时发生,但是最好的成功模式是在一个团队成形时就开展审查。你从事某个项目的时间越长,所创建的代码质量就越好。这是因为所有的代码审查过程和管理在项目开始时就开始了。

2. Team Code Review Practices

2.1. 负责人

Team Lead主持本团队的代码审查工作,确保此项工作有效的执行

2.2. 审查时机

pre-merge的具体做法:先创建一个审查区,把修改的代码放进去,然后讨论、等待批准、合并代码(辅助系统会自动合并代码)。

发布给测试的必须是讲过代码审核后的。

2.3. 审查粒度

审查是基于被修改的相关代码,一个单一审查可能会涉及很多次代码提交,审查的批准和拒绝应该是整体的。

建议实现一个新功能时分成三个步骤:

  1. 重构现有的代码,让代码整洁,方便添加新功能;
  2. 加入新功能;
  3. 写单元测试代码。

如果能将多个不同的提交放的一个代码审查中进行,那你就可以简单的将这些修改分组提交。

版本控制系统的最重要的功能就是告诉你代码演变的历史、是如何变成今天这个样子的。我经常会查看昨天代码是什么样的,上周二下午2点代码是什么样的,期间发生了什么变化。很多时候是因为我发现代码以前好用而现在不行,我想知道为什么。而更多时候,我是想知道为什么会对代码做这样的修改。关联的上下文是什么?动机是什么?

2.4. 评审开销

2.5. 审查规范

2.5.1. 编码规范

  1. 编码规范
    • 没有被使用的变量要删除;
    • 针对变量,方法和类要用相同的命名方法;
    • 常量应该被写在独立的常量类中;
    • 每行代码的尾部不要有多余的空格;
    • 对于括号,循环,if语句等等要用统一的格式;
    • 每一个单独的方法不应该超过100行;
    • 一个单独的语句不应该超过编辑器的可视区域,它可以被拆分成几行;
    • 假如类有很多成员变量,并且实例化的时候只需要少数变量传入的话,最好使用静态工厂方法,而不是重载构造函数。
    • 给方法添加适当的访问控制,而不是所有都是 public。
    • Java项目:
      • 检查 String 对象既不是null也不是空的最好方法是 if(“”.equals(str));
      • 针对不同的 Exception 要用不同的 catch 语句,而不是一个 Exception 解决所有问题;
      • 遵守项目中使用的框架的最佳实践建议,例如 Spring,Struts,Hibernate,jQuery;
  2. 注释
    • 每一个类和公共方法都要进行说明;
    • 如果是修复某个 bug,应该添加 bug ID;
    • 走捷径的方法或者复杂的逻辑要有解释;
    • 如果代码会被公开,每个文件头都要标注版权信息;
    • 复杂的脚本应该包含文档
  3. 功能
    • 如果类似的逻辑被使用达到了3次及以上,应该把它写成一个帮助类,然后在多出调用;
    • 鼓励使用 API 而不是重复编写代码解决相同的问题;
    • 要强调代码的单元测试;
    • 任何新加的代码不应该破坏已有的代码;
  4. 安全
    • 任何代码都不能直接执行用户的输入,除非经过转义过了--这个常常包含 JavaScript 的 eval 函数和 SQL 语句;

    • 任何类,变量,还有方法都应该有正确的访问域;
  5. 性能
    • 所有数据库和文件操句柄等资源在不需要的时候都应该被释放;
    • SQL 语句的写法会导致性能千差万别,需要分析线上真实情境进行对应优化(数据规模、操作类型和频度等);
    • 鼓励创建不可变(immutable)的类;

2.5.2. 日志规范

统一日志API:提供一个log输出类,将log输出格式及API统一

级别:输出日志的重要程度,便于控制日志输出的多少

输出内容:

log实例如下:

在什么位置打印日志:

The Problem With Logging 程序打日志需要注意的问题

2.5.3. 单元测试规范

针对以下部分必须有单元测试:

客户端:使用Android Unit

服务器端:使用JUnit

2.5.4. 代码提交规范

3. Peer Code Review Practices(with Gerrit)

3.1. Gerrit Workflow

Gerrit_Workflow.png

3.1.1. One Branch One Feature

Master branch contains only reviewed and approved changes

Each feature branch is based on master branch

A change can really be abandoned because

3.2. Code Review Tools

4. Best Practices

4.1. Code Review Tools

在进行代码评审之前,可以使用一些静态代码检查工具对代码进行初步校验

支持“批量提交审查”的软件和服务

4.2. Code Review Reference

Code Review 是一种主观性评价,跟工程师自身经验有极大的关系。

Code Review 和编码也是一种社交活动,总是一个人闷头写代码,缺乏沟通反馈,代码能力也很难提高

下面我将列出一些重要的评价指标。

4.2.1. Readability

代码被我们阅读的次数远远超过被编写和执行的次数,代码的可读性是评价代码质量最重要的指标之一。

我们在编写代码的时候,时刻要考虑到代码是否易读、易理解。除此之外,代码的可读性在非常大程度上会影响代码的可维护性。毕竟,不管是修改 bug,还是修改添加功能代码,我们首先要做的事情就是读懂代码。代码读不大懂,就很有可能因为考虑不周全,而引入新的 bug。

该如何评价一段代码的可读性呢?

我们需要看代码是否符合编码规范、命名是否达意、注释是否详尽、函数是否长短合适、模块划分是否清晰、是否符合高内聚低耦合等等。你应该也能感觉到,从正面上,我们很难给出一个覆盖所有评价指标的列表。这也是我们无法量化可读性的原因。

实际上,code review 是一个很好的测验代码可读性的手段。如果你的同事可以轻松地读懂你写的代码,那说明你的代码可读性很好;如果同事在读你的代码时,有很多疑问,那就说明你的代码可读性有待提高了。

4.2.2. Maintainability

所谓“代码易维护”就是指,在不破坏原有代码设计、不引入新的 bug 的情况下,能够快速地修改或者添加代码。所谓“代码不易维护”就是指,修改或者添加代码需要冒着极大的引入新 bug 的风险,并且需要花费很长的时间才能完成。

从正面去分析一个代码是否易维护稍微有点难度。不过,我们可以从侧面上给出一个比较主观但又比较准确的感受。如果 bug 容易定位、修复,修改、添加功能能够轻松完成,那我们就可以主观地认为代码对我们来说易维护。相反,如果修改一个 bug,修改、添加一个功能,需要花费很长的时间,那我们就可以主观地认为代码对我们来说不易维护。

是否易维护本来就是针对维护的人来说的。不同水平的人对于同一份代码的维护能力并不是相同的。对于同样一个系统,熟悉它的资深工程师会觉得代码的可维护性还不错,而一些新人因为不熟悉代码,修改 bug、修改添加代码要花费很长的时间,就有可能会觉得代码的可维护性不那么好。这实际上也印证了我们之前的观点:代码质量的评价有很强的主观性。

4.2.3. Testability

代码的可测试性也是非常重要的代码质量评价标准。代码可测试性的好坏,能从侧面上非常准确地反应代码质量的好坏。代码的可测试性差,比较难写单元测试,那基本上就能说明代码设计得有问题。

4.2.4. Simplicity

有一条非常著名的设计原则,你一定听过,那就是 KISS 原则:“Keep It Simple,Stupid”。这个原则说的意思就是,尽量保持代码简单。代码简单、逻辑清晰,也就意味着易读、易维护。我们在编写代码的时候,往往也会把简单、清晰放到首位。

很多编程经验不足的程序员会觉得,简单的代码没有技术含量,喜欢在项目中引入一些复杂的设计模式,觉得这样才能体现自己的技术水平。实际上,思从深而行从简,真正的高手能云淡风轻地用最简单的方法解决最复杂的问题。这也是一个编程老手跟编程新手的本质区别之一。

4.2.5. Reusability

代码的可复用性可以简单地理解为,尽量减少重复代码的编写,复用已有的代码。

代码可复用性跟 DRY(Don’t Repeat Yourself)这条设计原则的关系挺紧密的。

4.2.6. Extensibility

功能的可扩展性也是一个评价代码质量非常重要的标准。它表示我们的代码应对未来需求变化的能力。跟可读性一样,代码是否易扩展也很大程度上决定代码是否易维护。那到底什么是代码的可扩展性呢?

代码的可扩展性表示,我们在不修改或少量修改原有代码的情况下,通过扩展的方式添加新的功能代码。说直白点就是,代码预留了一些功能扩展点,你可以把新功能代码,直接插到扩展点上,而不需要因为要添加一个功能而大动干戈,改动大量的原始代码。

关于代码的扩展性,相关的理论主要就是 开闭原则单一职责原则

4.2.7. Scalability

服务的可伸缩性。

4.3. LinkedIn

LinkedIn’s Tips for Highly Effective Code Review

上文是 LinkedIn 介绍的内部评审实践,其代码评审技巧概括如下

5. 敏捷开放的更多实践

5.1. 结对开发

5.2. 测试推动的开发

5.3. 快速迭代

5.4. 每日晨会

6. Reference


CategoryProcess

MainWiki: Code_Review (last edited 2019-08-11 00:01:37 by twotwo)