Code Review 我们看什么

Code Review 中我们应该 review 什么

Louis Huang

3 minute read

Code Review 我们看什么

代码审核并不是指一个上级对于下级的工作进行审核的过程,而是一个项目组中各个成员互相了解对方和学习对方的一个机会。代码的审核行为并不能表现出级别的高低,因为对于项目中的某个特定模块来说,总有人是特别熟悉的,很有可能他就会发现针对这个模块一些不合理的操作。

写代码是一件不容易的事情,但是审核代码更加的不容易,我们提出了 merge request 的工作流程就是为了在工具上帮助大家降低代码审核的难度。我希望我们的技术团队在任何代码合并到主干前,都能有第二双眼睛看过,这样不仅仅可以缩短我们项目稳定周期的时间,还能给大家节省出更多的时间去做一些有意义的事情。

既然代码审核是一件有难度的事情,这篇文章主要用来总结我觉得应该在审核过程中需要注意的事情。

编码角度

  1. 能不能编译通过 一些非常明显的错误,基本上编译器或者是编辑工具都可能帮助你发现问题。当然这些问题理应不是审核者应该主要关注的方面,提交者进行代码提交时就应该处理好的问题,不过人总是会犯错的,特别是那些在最后一分钟的提交、跳过了正常的测试流程,最容易包含这样的问题。
  2. 有没有可见的BUG 是不是有一些一眼就能看到的问题?
  3. 功能或修正是否完整 是不是某些极端情况没有考虑到,或是在某些特殊的输入下会出现不正常的表现?
  4. 是不是好的解决方案 是不是你能想出来的各种解决方案中最优的那一个?
  5. 是否引入了新的安全问题 特别是对于新入职的同学,这是最常见的问题,某些函数没有做好判断,某些端口暴露到了外网,某些服务没有设置密码等等。
  6. 代码是否有性能问题 对于一些特别危险的函数,例如阻塞的函数,或是产生用户态和内核态切换的函数,在使用时是特别需要注意的,例如对于服务器来说,Lua 的一系列系统级别的函数都是不可以调用的,这样产生的性能问题可以在审核中很容易发现。
  7. 是否有风格问题 代码对于一个组织来说,统一的风格有助于其他成员的阅读,风格虽然没有好坏,但是一个组织内部的项目统一一个风格是有好处的,审核时应该对于不符合的代码风格产生异议。

自动测试方面

  1. 这段代码是否需要测试 有些时候,一些代码的修改非常的简单直白,例如更改一个 SDK 的 ID 字段等等,的确不需要更新什么测试代码。
  2. 有没有测试 说真的,测试对你对大家都有好处,需要把测试补齐,并且尽量做到最大的测试覆盖率。对于现在还没有单元测试的项目,需要尽快的添加。
  3. 自动化测试是否通过 没有通过自动化测试的提交是不可以合并的

整体感觉

  1. 是否某些功能重复 可能你发现这段代码中的一些逻辑已经和之前的某些功能重合了,这些功能应该提取出来做成独立的函数库,供大家使用
  2. 是否有漏洞 不论在实现上,还是策划的角度,是否有漏洞,例如服务器相信了客户端发上来的数据,或是玩家可以任意的刷钱或是复制。因为只要有漏洞在,用户总是可以找到。
comments powered by Disqus