Differences between revisions 3 and 4
Revision 3 as of 2016-12-15 20:38:22
Size: 16521
Editor: twotwo
Comment: 基于Gerrit的代码评审实践
Revision 4 as of 2019-08-11 19:01:37
Size: 18855
Editor: twotwo
Comment: LinkedIn 高效的代码评审技巧
Deletions are marked like this. Additions are marked like this.
Line 143: Line 143:
[[https://blog.codinghorror.com/the-problem-with-logging/|The Problem With Logging|target="_blank"]] 程序打日志需要注意的问题
Line 213: Line 215:
== 更多实践 == === LinkedIn ===
[[https://github.com/rietveld-codereview/rietveld/|LinkedIn’s Tips for Highly Effective Code Review|target="_blank"]]

上文是 !LinkedIn 介绍的内部评审实践,其代码评审技巧概括如下
 * Code Review 的一个难点是,Reviewer 可能不了解某块代码修改的背景和目的。所以 !LinkedIn 要求代码签入版本管理系统前,就对其做清晰的说明,以便复查者了解其目的,促进 Review 的进行。我认为,这个方法实在太赞了。因为,我看到很多时候,Reviewer 都会说不了解对方代码的背景或是代码量比较大而无法做 Code Review,然而,他们却没有找到相应的方法解决这个问题。!LinkedIn 对提交代码写说明文档这个思路是一个非常不错的方法,因为代码提交人写文档的过程其实也是重新梳理的过程。我的个人经验是,写文档的时候通常会发现自己把事儿干复杂了,应该把代码再简化一下,于是就会回头去改代码。是的,写文档就是在写代码。
 * 有些 Code Review 工具所允许给出的反馈只是代码怎样修改以变得更好,但长此以往会让人觉得复查提出的意见都表示原先的代码不够好。为了提高员工积极性,!LinkedIn 的代码复查工具允许提出“这段代码很棒”之类的话语,以便让好代码的作者得到鼓励。我认为,这个方法也很赞,正面鼓励的价值也不可小看。
 * 为 Code Review 的结果写出有目的性的注释。比如“消除重复代码”,“增加了测试覆盖率”,等等。长此以往也让团队的价值观得以明确。
 * Code Review 中,不但要 Review 提交者的代码,还要 Reivew 提交者做过的测试。除了一些单元测试,还有一些可能是手动的测试。提交者最好列出所有测试过的案例。这样可以让 Reviewer 可以做出更多的测试建议,从而提高质量。
 * 对 Code Review 有明确的期望,不过分关注细枝末节,也不要炫技,而是对要 Review 的代码有一个明确的目标。

== 敏捷开放的更多实践 ==
Line 240: Line 252:
 * [[BookWiki:geekbang_048_ChenHao_ARTS|陈浩的技术分享]]

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:在代码进入主代码库之前进行代码审查;

  • post-commit:在代码进入主代码库之后审查

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

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

2.3. 审查粒度

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

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

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

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

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

2.4. 评审开销

  • 代码检查的总体开销为,编码和评审之比大致5:1;
  • 一次评审量时间不要超过60分钟,选取评审的内容不要超过 500 LOC(行代码);

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统一

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

  • 通常可以分为4级: DEBUG > INFO > ERROR > NONE

    • DEBUG:调试信息(SQL语句、类详细信息等)
    • INFO:关键业务信息(收发业务数据,统计业务数据等)
    • ERROR:错误信息
  • Log的级别是可以配置的。当总的log级别是ERROR时,则只有 ERROR信息才会被输出

输出内容:

  • 日期时间
  • log级别
  • 类名+行数
  • log信息(服务器端必须输出Client的唯一标示,例如UUID)

log实例如下:

  • 服务器Log输出形式如下(服务器端使用 log4j):
    • 2014-11-19 11:31:29   INFO   DaoObject       line: 22 getTime:0

  • 客户端将Android 原生log机制封装成一个LOG类。Log输出形式如下:
    • 2014-11-19 11:31:29   i      ConversationListAdapter line: 22 thread Id: 1 getTime:0

在什么位置打印日志:

  • 对程序的输入输出要以INFO记录下来。通常包括从文件、数据库、网络、用户等输入的信息及向文件、数据库、网络输出的信息
  • 对重要对象或实体的修改,要以INFO记录修改前后的状态
  • 在程序开始运行和初始化完成时,应以INFO记录信息
  • 程序运行过程中,出现错误流程,错误逻辑或异常,应以ERROR记录
  • 在函数的入口,如果需要验证参数,则以INFO形式输出参数信息。如果重要参数不正确,则应该以ERROR输出
  • 对程序中的每个线程,它们的初始化完成、开始运行和结束的时候也要以INFO记录下来。

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

2.5.3. 单元测试规范

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

  • 持久层
  • 重要service
  • 协议解析和处理
  • 重要算法
  • 重大Bug复现

客户端:使用Android Unit

服务器端:使用JUnit

2.5.4. 代码提交规范

  • BUG修改完成,本地必须验证通过
  • 提交代码时,必须写注释。如有JIRA编号,写明JIRA编号和JIRA标题。尽量写明问题描述及修改原因
  • JIRA里关闭BUG,需要注意以下几点:
  • 必须写明SVN提交号
  • 对BUG如何解决做简单的描述
  • 如果针对此BUG有分析讨论,也请提供相应描述
  • 写明本次提交可能对系统产生的影响

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

  • master moves from good to better state after each (approved) change

Each feature branch is based on master branch

  • stable starting point

A change can really be abandoned because

  • no other approved change can depend on a not yet approved change
  • Gerrit will automatically reject a successor change of an abandoned change

3.2. Code Review Tools

  • Gerrit
  • Eclipse(with EGit)

4. Best Practices

4.1. Code Review Tools

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

此外,有专门的辅助代码审查的工具可用应用到代码审查过程中来,例如Gitlab

支持“批量提交审查”的软件:

4.2. Review Board

Review Board是一个 基于web 的工具,是由 django 和python设计的。 Review board 可以帮助我们追踪待决代码的改动,并可以让Code-Review更为容易和简练。尽管Review board 最初被设计在VMware项目中使用,但现在其足够地通用。当前,其支持这些代码版本管理软件: SVN, CVS, Perforce, Git, Bazaar, 和Mercurial.

➜  reviewboard  virtualenv env
New python executable in env/bin/python
Installing setuptools, pip...done.
➜  reviewboard  source env/bin/activate
(env)➜  reviewboard  pip install reviewboard nose Sphinx
(env)➜  reviewboard  wget http://pysvn.barrys-emacs.org/source_kits/pysvn-1.7.2.tar.gz 
## tar && cd Import
wget http://sourceforge.net/projects/cxx/files/CXX/PyCXX%20V6.1.1/pycxx-6.1.1.tar.gz

4.3. Rietveld

Rietveld 由Guido van Rossum 开发(他是Python的创造者,现在是Google的员工),这个工具是基于Mondrian 工具,作者一开始是为了Google 开发的,并且,它在很多方面和Review board 很像。它也是一个基于Web的应用,并在Google App Engine 上。它使用了目前最流行的Web开发框架 django 并支持 Subversion 。当前,任何一个使用 Google Code 的项目都可以使用 Rietveld 并且使用 python Subversion 服务器。当然,它同样支持其它的Subversion服务器。

4.4. LinkedIn

LinkedIn’s Tips for Highly Effective Code Review

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

  • Code Review 的一个难点是,Reviewer 可能不了解某块代码修改的背景和目的。所以 LinkedIn 要求代码签入版本管理系统前,就对其做清晰的说明,以便复查者了解其目的,促进 Review 的进行。我认为,这个方法实在太赞了。因为,我看到很多时候,Reviewer 都会说不了解对方代码的背景或是代码量比较大而无法做 Code Review,然而,他们却没有找到相应的方法解决这个问题。LinkedIn 对提交代码写说明文档这个思路是一个非常不错的方法,因为代码提交人写文档的过程其实也是重新梳理的过程。我的个人经验是,写文档的时候通常会发现自己把事儿干复杂了,应该把代码再简化一下,于是就会回头去改代码。是的,写文档就是在写代码。

  • 有些 Code Review 工具所允许给出的反馈只是代码怎样修改以变得更好,但长此以往会让人觉得复查提出的意见都表示原先的代码不够好。为了提高员工积极性,LinkedIn 的代码复查工具允许提出“这段代码很棒”之类的话语,以便让好代码的作者得到鼓励。我认为,这个方法也很赞,正面鼓励的价值也不可小看。

  • 为 Code Review 的结果写出有目的性的注释。比如“消除重复代码”,“增加了测试覆盖率”,等等。长此以往也让团队的价值观得以明确。
  • Code Review 中,不但要 Review 提交者的代码,还要 Reivew 提交者做过的测试。除了一些单元测试,还有一些可能是手动的测试。提交者最好列出所有测试过的案例。这样可以让 Reviewer 可以做出更多的测试建议,从而提高质量。
  • 对 Code Review 有明确的期望,不过分关注细枝末节,也不要炫技,而是对要 Review 的代码有一个明确的目标。

5. 敏捷开放的更多实践

  • 敏捷开发中和保证代码质量有关的实践有很多,我觉得在项目经理或研发经理、组长们可以学习一下,挑一些在自己负责的开发工作中做一下实践。
  • 咱们推行的代码质量审核,是要求编码者的上级来审核代码,这和敏捷开发中的结对编码,一个人写完的代码,要讲给另外一个人,请他来看。目标是一样的,但执行上结对编程更能激发团队成员的积极性。

5.1. 结对开发

  • Pair programming is a software development technique in which two programmers work together at one keyboard. One types in code while the other reviews each line of code as it's typed in. The person typing is called the driver. The person reviewing the code is called the observer or navigator. The two programmers switch roles frequently.
  • 结对开可以用来培养新人,提高代码质量。经典的结对编程是指两人共用一台电脑,一人编码,一人查看,实时沟通。结对编程是共同分享技术挑战和难题,共同享受编码的乐趣。这会显著降低程序员的工作枯燥程度,提高技术水平。结对已经成为很多技术团队的主要的培训方式。
  • 但我们也应该看到,结对开发对开发资源,还是有一定要求的。在实际实施中,在资源允许的情况下,我们可以安排一人编码。当编码结束后,再引入另外一个人。由编码者向新人讲解代码改动原因,改动地方,请新人检查改动的代码。逐步实施Pair Programming标准,保证代码质量。

5.2. 测试推动的开发

  • 所有项目都要求以TDD的方式开发,要求先编写测试代码,然后再编写功能代码,使得这个测试被通过。新程序员入职最先接触到的就是TDD的知识和各种培训。我们大量采用了Autotest这种工具,使得测试代码在开发过程中持续运行。甚至为了更方便查看,每个成员都配备了第二个显示器,其主要用途就是为了显示测试运行结果。

5.3. 快速迭代

  • 敏捷开发中讲究短周期的迭代,尽量缩短项目Release的时间。通过自动集成工具做到每天自动部署到测试服务器上,方便远程的客户直接在互联网上查看当天的产品进度,并及时提交反馈。

5.4. 每日晨会

  • 每日晨会上,大家回答“昨天做了什么工作?”、“今天计划做什么工作”。早上项目组的人到齐后,第一件事就是站成一个圆圈,依次讨论昨天的工作和今天的计划。期间也有很多关于项目的整体需求,技术难点的讨论。由于大家都是站立开会,会议鼓励简短,高效的发言,显著提高了项目组的内部沟通和信任,每日晨会的帮助很大。
  • 从我们的实践经验来看,执行力无疑是实施过程中很重要的因素。任何流程的形成都需要很大的决心和长期的努力。我们的经验是,一项一项地进行实施。实施过程中不断进行调整和反省,当TDD成为一种习惯后,便开始晨会,几个星期后,晨会也成为团队习惯了,就能进行下一步。需要团队中有人强力地坚持推行敏捷思路,而且要慢慢推进,一项一项地推进。

6. Reference


CategoryProcess

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