我们应该如何进行 Code Review ?

笔者在日常工作中会发现，时常会存在一些比较显而易见或者已知的一类问题，在开发者提交代码以及相关同学 Review 中均未被发现。

这里的一个原因在于，代码开发者在某些时候会提交较多代码，而此时 Reviewer 面对较多代码的 Review 的时候，很可能会漏掉一些 Review 的要点，甚至可能会感到无所适从。本文希望，针对一些非场景化的内容，整理出一些较为普适的一些原则，从而帮助 Reviewer（以及开发者本身）解决一些通用的问题。

格式与规范

原则上，这部分内容不应该在 Review 的时候去处理，我们应该配置代码格式规范工具，在提交代码前或者 MR 的 CI 流水线中做这个事情。

这里对于不同类型的语言，大多社区都有现成的方案，不过多赘述，如果你在代码 Review 的时候还在纠结于格式和规范，那么现在就去配置好对应的工具。

稳定性

这里的稳定性，指的是我们程序在运行阶段的稳定性，以及当出现错误，我们能够第一时间发现。

错误处理

关于这里，最常见的一种代码是：

if (some_error_case) {
    return;
}

对于这种类似的代码，虽然看似兜底兜住了错误，但是并没有日志输出，也没有更多的提示，长久之后很可能会形成暗坑。

对于这种代码我们应该思考：

• 是否应该打一个错误日志，提供一些上下文信息。
• 是否应该增加异常上报，让我们优化这种问题。
• 是否需要将这个错误信息返回给调用方或者调用端。
• 用户层面是否需要感知此信息，如果需要，方法是什么。

另外还有一种比较常见的是异常，包括代码运行抛出的异常和我们自己捕获/抛出的异常：

if (some_error_case) {
    throw new Error('some error');
}

try {
    error_call()
} catch(e) {
    log(e)
}

这类异常的处理逻辑，和上面的异常条件逻辑比较类似，我们应该同样思考上述四点。

保护用户隐私

隐私通常涉及到法律合规，因此可能是最重要的一环。

用户的隐私不可泄漏

• 比如在开发功能打日志的时候，我们应该不能泄漏用户的隐私，例如用户的自定义信息、聊天信息等。

这一部分建议在团队内部形成一个统一的规范文档，在开发和 Review 的过程中，大家都可以以此为依据。

自己的隐私不可泄漏

很多时候，我们虽然注意到了用户的隐私，但是自己的隐私却没有注意，这里主要是我们的一些 appKey、密钥等内容。无论是在以下哪些场景，我们都需要格外注意：

• 对产物进行二次分发，例如分发到内部或外部 npm、github 以及其他团队，特别是外网可访问的情况更需要注意。
• 直接给到用户使用，例如在 Web 环境使用，或者打包到桌面端应用，尽量避免用户直接接触到这些内容。

可读性/维护性

命名

代码中函数和变量的命名，是可读性中比较重要的一个关注点。

关于如何命名，实际上很多架构书籍也都会额外列出一章来讲，在这里我不想过多进行赘述。不过一般来说，我们的命名应当清晰易懂，避免出现没有实际意义的含糊的变量名。

代码提交的注释

• 对于 bug fix：很多时候我们是在处理一个边界问题，或者某种兜底，这个时候 fix 的代码可能比较难以理解，在这种情况下我们最好是写明注释，如果有相关的文档和 bug 单，我们也可以一并贴到注释中。另外，如果我们的 bug fix 是一个临时方案，请在代码中写明 TODO，来提醒自己和别人这个地方后续还会继续更改。

• 对于新功能添加：一般我们需要在关键入口处写明功能的说明文档链接、完善相关关键路径的注释，同时删除功能开发中的冗余代码：例如我们在开发过程中测试用到但是最终却没有用到的大段注释掉的代码，以及大段的注释和 TODO，我们都需要在最终提交的时候删除掉，如果需要找回建议使用 git 的能力，可以适当进行注释备注。

• 另外，对于注释来说，禁止出现模糊的词汇，例如 感觉，好像，大概 等这种模糊的词汇，而是要培养自己严谨的意识，已经提交的代码必须有严格的佐证。

日志可读性

大多数项目的问题定位非常依赖日志。

对于日志，我们应该有如下几个约束：

• 避免太多太长的内容，无论日志是以何种方式存储，规模上去之后的消耗都非常可观。
• 适当使用简写，但是避免完全不知所云，应该符合普遍约定。

代码的通用性

当我们提交一个功能，或者修复一个问题的时候，很多时候我们只是从这个问题的角度护发，但是并没有从全局的角度出发，例如：

• 当我们新增一个上报的时候，有没有考虑把整个上报聚合到一起，或者架构上支持更方便的上报能力？
• 当我们新增一个通用能力的时候，有没有考虑到其他模块或项目也可能有类似需求，我们是否可以将其单独抽离成一个独立的包进行分发？
• 当我们遇到一个问题有多种修复方案的时候，有没有综合考虑，哪一种方案对后续维护的同学更加友好（比如，最好是高内聚、低耦合的设计）？

这里的大体原则，就是我们需要从整体性的角度出发，不断地迭代让整个架构更加夯实，而不是出个问题贴一个创可贴，新增功能又贴一个创可贴。

配置化

这里的配置化，比较典型的比如是：多语言文案、项目 Settings 配置等。

• 一般来说，配置化的内容最好是走云端下发。
• 如果不具备云端下发的环境，或者我们的配置比较敏感不适合直接下发，可以考虑在代码中创建配置文件的方式。

如果我们在 Review 中发现有可以配置化的内容但是却直接写死在代码里面了，应当需要提出质疑。

代码精简

代码精简对后期的可维护性是非常重要的，代码精简的一个比价有效的办法就是充分理解业务，写出精简不多余的代码，不过这一点在 MR 中可能会比较难进行 Review，因为一般来说提交代码的同学本身已经是对这部分业务是理解的最透彻的。

但是有一点代码的 Reviewer 会比较容易判断，如果我们提交的代码里面有两处以上超过三行的极其相似的代码，我们就应当重新审视是否可以进行一定程度的抽离，建议不要由于一时的效率允许不符合规范的代码合入，这些在后期都可能演化成代码屎山。

例如，笔者对于 Rust 代码精简的一些建议：

• 避免任何一次多余的 Clone。
• 文件头部引用中去除没有必要的引用。
• 避免多余的日志，同时避免在日志里面加太长的前缀内容（有的时候一行日志里面有多半都是元信息，而且这些元信息还是有所重复的）。
• 避免多余的 pub，应该有一个理念就是默认的内容不要 pub 出去，就像 c++ 的成员方法默认是 private 一样。
• 重复代码使用宏来替代。

重视性能

防止泄漏

在考察性能之前，我们需要保证自己的代码没有泄漏，因为泄漏造成的恶劣性通常比性能差更严重，而且通常需要更长的时间来排查。

这里主要的检查点可以是：

• addListener 之后是否及时释放了。
• setInterval/setTimeout 等定时器调用是否存在多次调用的可能性，以及是否可能无法释放。
• 分配在堆上的内存是否释放了。
• 是否有 detach dom 泄漏。

防止死锁

这一点如果是前端开发，一般没有机会遇到，但如果你用 rust、c++ 等语言，都很有可能出现死锁的风险（严格来说，死锁并不能直接归类到性能）。

对于死锁的防止，我建议团队内的基础设施部分先配置好死锁检测和上报机制（例如 parking_lot 提供了死锁检查的能力）。

一些死锁相关的 Review 建议：

• 用到锁的地方，尽可能通过工具函数进行封装，类似 getter 和 setter，增加原子性，减少调用代码直接解锁的场景。
• 锁的粒度不应该太大，我们应该是对数据进行加锁，而不是对过程进行加锁，锁粒度过大很容易出现死锁的风险。
• 在函数脱离控制权之前，例如准备开始调用到其他外部函数了，这个时候最好把持有的锁都释放掉，防止外部函数再次用到造成锁重入。

性能报告

关于这一部分，也建议团队基建先行，有一个比较标准的性能测试方法，这样大家在做性能测试的时候，不会那么有压力。比较反对的一种方式就是团队内部不同成员都有自己的一套性能测试方法，这样有些新同学缺乏必要的上下文，自然不了解如何去做性能测试。

一般来说，我们涉及到比较大的功能都要进行一些性能报告，相关的指标可以是：

• 整体包体积的增加幅度。
• 初始化响应速度的变化。
• 运行该功能一段时间的 CPU 消耗。
• 运行该功能一段时间的 内存 消耗。
• 运行该功能较长时间的 CPU/内存 消耗。

不过，具体这一部分还是建议团队内部根据项目的实际情况有一个统一的标准，并且将性能测试方法标准化，而不要成为一个负担。

兼容性

在兼容性这方面，无论是 web 开发还是桌面客户端开发，都需要重点关注，这里其实细分为两个维度：

• 版本支持，例如我们可能会实现约定好支持到 Chrome 的哪个版本（针对 web）或者 macOS 的哪个版本（针对 macOS 桌面端），并且如果提交涉及到兼容性改动需要重点测试所支持的最低版本。
• 异常情况支持，这一点可能大多数时候都会被大家忽略，这里举一个例子就是 webgl 的支持，实际上，虽然大多数时候浏览器已经支持到了 webgl，但是仍然有不少场景，webgl 会初始化失败，这个原因可能和用户的硬件比如系统显卡有关，如果我们的项目只有 webgl 实现，我们需要考虑是否放弃 webgl 初始化失败的场景（从商业化的角度，一般来说都是希望不要放弃），或者说为此增加软渲染降级。

所以，在 Code Review 环节，针对可能出现兼容性风险的地方，我们需要确认是否已经进行了兼容性测试。