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