编写可 Review 的代码

programming  review 

总览

本文档介绍了一种结构化的代码管理策略，该策略在各类开源项目中都有实践（例如 Linux、React、Vue)，简而言之的话有以下几个原则：

  1. 每个提交（commit）应尽可能小；
  2. 每个提交的提交都要是一个内聚的实现；
  3. 通过将大问题分解为较小的问题并一次解决一个小问题，最终将大提交变成小提交；
  4. 编写清晰的提交消息（commit message）；
  5. 代码中在关键位置要保留足够的上下文；
  6. 使用无副作用且简洁的实现；
  7. 编写足够的测试用例。

提交还是小的好

小型、简单的提交通常比大型、复杂的提交要好。它们更容易理解，更易于测试和更易于查看。理解、测试和审查变更的复杂性通常比变更的大小要快：每次做一件事情的十个200行的变更通常比做十件事的一个2000的行变更更容易理解。

将一个可以做很多事情的更改拆分为每个只能做一个事情的较小更改，可以减少实现同一目标的总复杂度。

而且，即使出现问题，如果只有一个提交，也会很容易进行回滚（Revert）。

每个提交的提交都要是一个内聚的实现

每次提交都应该只实现一个功能或者修复一个问题。通常，这意味着在开发时应将不同的更改分为不同的提交。例如，如果您正在开发功能并遇到先前存在的错误，请清理出干净的 HEAD，修复该问题，然后在此基础上进行新功能开发，这样您就可以进行两项更改，每项更改都有一个单独的提交（修复Y中的错误、添加功能X），而不是一项更改有两个提交（添加功能X并修复Y中的错误）。

将大的问题分解为小的解决方案

同样，将复杂的更改分解为更小、更简单的更改会更好。举个栗子：如果要建设一栋新房子，请不要一次性把整个房子全部完工后直接交付。将其拆分为每个易于理解的较小步骤：从地基开始、然后构建支撑、楼层、外装、室内设计等等，修依次慢慢来，做完一点、检查一点、交付一点。

如果您决定用铲子挖地基或用硬纸板构建框架，则既容易出错，也难以纠正可能埋在地底的误差。每次执行一点，提供足够的上下文，可以让整个架构更加容易理解，每个步骤仅完成所需的步骤即可，并需要使它具备足够的独立性，别的施工跟它无需耦合即可运行。

更改的最小体积应该是最简单的子问题的完整实现，该子问题可以独立工作并表达整个设计，而不仅仅是设计的一部分。您任意将1000行的更改分为十个100行的更改，但是这10个100行的更改可能没有任何意义，并且会增加总体复杂性。真正的目标是使每次更改具有最小的复杂度，行大小通常与问题和设计的复杂度紧密相关。

编写清晰的提交信息（commit message）

最重要的一点是：提交消息应说明您进行更改的原因。

自动化检查能够检查提交消息，但只能强制执行结构，而不能强制执行内容。从结构上讲，提交消息可能应该：

  1. 标题，在一行中简要描述更改。
  2. 总结，以更详细地描述更改。
  3. 也许还有其它信息。

内容远比结构重要。特别是，摘要应说明为什么要进行更改以及为什么要选择要实现的实现。变更本身通常可以很好地解释变更的内容。举个栗子，下面是个完全没有意义的提交消息：

修复了一个 bug

下面这个也好不到哪儿去：

增加了一个正则表达式限制用户名只接受 ASCII 和数字

其实比较好的提交信息是能告诉 Reviewer 这个提交解决了什么样的问题，跟之前代码的区别在哪里，并且提供一下为什么要这么做的上下文，以及这是正确的做法。

限制用户名只接受 ASCII 和数字

在之前的实现中并未对用户名加以限制，任意字符都可以通过申请用户，这导致跟其它系统进行交互时发生问题，例如创建用户文件目录时因为受文件系统限制创建失败。

这个补丁仅修改了前后端校验逻辑，通过在前端进行输入校验，对输入内容进行限制，避免了此类问题的发生。

具备上下文的注释信息

对于代码评审者而言，其实不怎么关心代码的作用是什么，对于绝大多数代码而言其实是可以读出来的，他们会更关心代码的上下文关系， 这段代码通过什么方式，解决了什么问题：

比如同样一段代码，等待 30 秒：

  // 推送镜像
  exec('docker push tag');
 
  // 等待 30 秒
  await sleep(3e4);

但是为什么要等待呢，却没有给出原因。但如果按照下面这样写，就很清晰了。

  // 推送代码后，因为仓库存在缓存，直接更新版本会出错，所以等待一下，稳妥起见等待 30 秒确保缓存刷新完成
  await sleep(3e4);

确保实现无副作用且简洁

副作用（Side effect）简而言之就是一段代码的实现有两种不同的返回值，阅读体验上会非常糟糕，很难一眼就看出这段代码的作用是什么，为什么不同条件还会有不同的返回值？这些不同的返回值又有何用？

从编译器角度而言，如果不同返回值的类型不同，也会造成优化困难，甚至可能进入 slow path。

比如下面有一段代码，用来找一段数组中，比输入值大的数就返回：

/**
 * 在数组里面找出第一个比目标数字索引值大的数，如果没有则返回 null
 */
function getLargerNumIndex(arr: number[], target: number): null | number {
  let index = 0;
  do {
    let current = arr[index];
    if (current > target) return index;
	    index += 1
  } while (true);
  return null;
}

这个函数有什么问题？粗看其实没什么问题，但是不同的返回值类型在使用它的函数里，就需要进行额外的判断。而且获取索引值的函数里会拿到 null 本身就是个反直觉的设定，一般都是拿到 -1 才是。

从执行层面来说，现代 JS 引擎会对这种情况进行优化，但是保持相同的返回值类型也能提升 JS 引擎的识别和执行效率，更少的类型判断代码也能提升执行性能。

所以第一步，就是将结尾的 return null 改为 return -1，这样就保持了返回类型的统一。

如果对 JavaScript 很熟悉的人，还有个更简单的办法是直接使用数组的 findIndex() 方法，直接一行就写完了，这就要求开发同学对数据结构的处理有扎实的基础，一行的代码所能表达的内容却比之前的多行内容一样，但是阅读负担就要少很多了。

function getLargerNumIndex(arr: number[], target: number): number {
  return arr.findIndex(n => n > target);
}

足够的测试代码

测试代码除了可以保证自身质量的保证意外，对于代码评审者而言，还是代码的使用说明书，很多情况下对于代码的审视都是从单元测试开始的，通过在不同场景内观察逻辑的不同行为和返回值就能知道逻辑的具体工作目标，所以测试代码写得好的话对代码评审者是种减负。

版权所有丨转载请注明出处：https://kxq.io/archives/编写可review的代码