PSJay Blog

#FIXME, seriously

Code Review 指南

| Comments

一开始这篇指南是为团队而写的,后来发现包含的内容比较有一般性,所以删掉了一些和团队强相关的内容,发到这里。


「Code Review」直译过来便是「代码复审」,也就是让代码作者之外的人来复审代码,提出修改意见,接受代码变更。

益处

  • 发现错误:一个人的思考总是会不可避免地出现一些纰漏,而这些纰漏在另一个人眼中也许显而易见。
  • 保持代码风格统一:对于整个团队来说,代码风格的统一显得至关重要。风格一致的代码能提供更好的可读性,也能避免犯一些「低级错误」。
  • 保证设计的合理性:代码直接反映出你的系统设计,在写一个新的系统时,确保你的设计不会出现大问题。
  • 保证提交的变更符合团队的其他要求:例如对测试用例的要求和对文档的要求;
  • 互相学习:Code Review 的过程也是思想交流的过程,可以取人之长,补己之短。

怎样进行 Code Review

Review 时机

基本的语法检查和自动化测试通过之后,代码被合并到主分支之前。

Reviewer

Reviewer 可以是一个,也可以更多。在代码变更越大、变更内容牵扯到更多的人和外部系统,或者自信度越低的情况下,你就可能需要更多的 Reviewer。另外在不同的场景下,你也需要不同角色的 Reviewer 参与。

  • 新人上手:Mentor 是第一选择;
  • 日常工作:同组的其他工程师;
  • 向其他系统或模块提交 Patch 或 Bugfix:相应系统或模块的 Maintainer;
  • 专业度很高的代码:团队中相应的领域专家。

内容

Reviewer 需要针对变更作以下复审:

  • 代码风格:确保变更代码风格符合规范。
  • 明显的错误:确保变更代码不出现低级错误,例如访问不存在的属性或访问空指针。
  • 不合理的设计:确保变更代码不出现明显的不合理的系统设计,例如单个 Redis 实例存储过多的数据、滥用 OOD 或动态语言特性。
  • 明显的性能问题:确保变更不会引发明显的性能问题,例如连续、大量、串行地调用 RPC 接口或者不合理的存储访问模式。
  • 测试用例:确保相应的变更有相应的单元测试。
  • 注释和文档:确保有必要的注释和接口说明文档。
  • 其他团队约定:确保变更的内容符合团队的其他约定,例如服务的接口的访问需要接入监控系统和日志系统等。

如果被 Review 的内容对应一个新的系统或功能,Reviewer 还需要确保:

  • 理解这个功能的细节
  • 理解这个系统的工程设计细节

合并时机

通常来说,需要所有的 Reviewer 都接受变更才能合并代码。

Review 文档

  • Python 代码规范
  • MySQL 使用规范
  • Redis 使用规范

编写 Review 友好的代码

高效地进行 Code Review 不仅仅只是 Reviewer 的工作,代码的作者也起着至关重要的作用。

Commit 粒度

总的来说,Commit 的粒度应该遵循以下原则:

  • 尽可能小:一个 Commit 的变更应该尽量少,一个 commit 只做一件事情,如果一件事情可以被拆分成多件事情,那么它们应该对应不同的 commit。例如,不要将「修复一个 Bug」和「重构一个函数」放到同一个 commit 里。
  • 自成单元:一个 Commit 应该自成一个单元,例如,不要将一个新增的函数放到多个 commit 里。

Commit Message

Commit Message 应该包含两部分内容:

  • Commit 的变更总结:变更内容本身就已经说明了详细变更,所以在 message 里面只需要一个变更总结。
  • 为什么要进行这次变更:这是更重要的一部分,因为在大多数情况下,变更内容并不能说明为什么需要做这次变更,知道变更原因才能更好地理解变更内容。只有当变更原因特别显而易见时才能省略此部分内容,例如修复一个不存在的 API 调用。

Commit Message 的推荐格式为:

1
2
3
变更内容总结

变更原因详细说明...

例如:

1
2
3
Refactor foo module with gevent.

We met performance issue with foo module recently, foo handles client requests one by one so we just use gevent for concurrency.

坏的例子,如:

1
Fix a bug.

这个也好不到哪儿去:

1
2
3
Update config.

Update the 'host' config to `b_server` from `a_server`.

Review 单元粒度

一个 Review 单元包含一个或多个 commit。

Review 单元的粒度应该遵循以下原则:

  • 尽可能小:一个人连续高度集中精力的时间通常不会太长,所以更小的 Review 单元能够得到 Reviewer 更细致的复审,Code Review 的质量就会更高。
  • 能够安全地并入主分支:Review 单元在被 Reviewer 接受之后应该尽快并入主分支,这样才能更好地实现持续集成和持续交付。因此,一个 Review 单元应该要能安全地并入主分支。

参考来源

Comments