Code Review最佳实践

因为最近在工作上参与制定了团队的一些Code Review(CR)的规范，所以想在这里给大家分享一下我们积累的一些CR最佳实践。本篇文章会包括下面这些内容：

•为什么需要Code Review

•什么时候做Code Review

•Committer需要注意什么

•Code Reviewer需要看哪方面的内容

为什么需要Code Review

对于参与人数大于或者等于两个的项目来说，CR是一项必不可少的活动。在我看来它有下面这些好处：

可以提高committer对于自己代码的要求

按照我以往的经验来说，有些人的代码在被review和不会被review的时候风格是完全不一样的。如果一个人写的代码不用别人看，永远都是直接推到或者直接合入主分支的话，这个人写代码的时候就不会有什么顾忌，自己想怎么写就怎么写，反正也不会有人看。可是当他的代码需要被同伴review时，他的心态就完全不一样了，这个时候他会想办法精简代码，去掉一些硬编码的逻辑从而追求一些最佳实践，而且各种TODO和FIXME也会写得一清二楚。因为他如果不怎么做的话，作为一个及格的code reviewer，我们是肯定不会将他的代码合入主分支的。长期如此的话，committer的代码水平也会逐渐得到提高的。

组内知识分享

俗话说的好：你有一个苹果，我有一个苹果，我们交换一下，一个人还是只有一个苹果；你有一个思想，我有一个思想，我们交换一下，一人就有了两个思想。这句话同样适用于我们进行软件开发。作为一个程序员，我们在面对同一个问题的时候，可能会有不同的解决方法。如果大家没有交流的话，每个人可能永远都只知道一种解决方法，大家也就没有进步可言。那么程序员之间如何互相学习呢？最简单的办法就是看别人的源代码，而CR就是最好的阅读别人源代码的过程。因为当我们在帮committer进行review的时候，他提交的代码只专注于解决一个问题，这样代码量就不会很大，我们可以更加清晰地学习到他是如何解决某一个问题的。如果committer恰好用了一个很巧妙的我们之前不知道的办法来解决问题的话，我们就可以学习到新的知识了。

保持项目代码风格的一致性

同一个项目里面不同的程序员由于背景不一样(可能来源于不同的项目，来源于不同的公司)，他们解决问题的方法和思路就不一样。作为一个前端程序员我见过在同一个代码仓库里面同时使用了redux-saga和redux-thunk作为异步中间件的。如果作为code reviewer我们不对committer的代码进行约束的话，项目的代码风格就会多极分化，这无疑会增加项目的维护成本以及后面新加入开发者的学习成本。

提前发现代码的问题

一些经验比较少的开发者在写代码的时候可能考虑问题不够全面，导致一些边缘情况(edge case)没有考虑到，这时候如果code reviewer是一个工作经验比较多的同学的话，就可以帮committer提前发现问题，这样就可以避免在产品上线后再浪费时间去定位问题了。值得注意的是，就算code reviewer不是一个很资深的开发，他作为代码的局外人往往也可以考虑到当事人(committer)在写代码时考虑不到的情况的。

什么时候进行Code Review

CR一定要发生在各种CI自动化检查例如单元测试，lint检查等通过之后和代码正式合入主分支之前。

这里涉及到的一个问题是如果我们开发的功能涉及的改动很多的话，是等我们都开发完后再一起给reviewer看呢还是拆分成一个个小的MR(Merge Request)给reviewer看呢？我个人偏向于后者。因为如果你提的代码很多的话，code reviewer需要花很多时间去阅读你的代码，理解各个模块之间是如何协作的，而且给你提问题后，你可能需要改动很多的地方，这就导致整个CR周期变得很长，这些其实都会打击code reviewer给你挑毛病的积极性，这种情况下，一些没有耐心的code reviewer可能就给你一个简单的LGTM然后就将你的代码合入主分支了，这样的CR是毫无意义的。因此更好的做法是在开发大功能时候将代码拆分成一个个小的模块，每完成一个小的模块就合入主分支，直到所有的功能都合并入主分支为止。可是如果开发者不想将自己未完成的功能模块合入主分支怎么办呢？这个时候可以使用一种叫做stacked CR的模式：首先从主分支切出一个feature/big-feature的分支，你后面在开发这个big-feature的时候，都是从这个feature/big-feature分支切出各种功能的小分支，例如feature/big-feature-sub-feature1，feature/big-feature-sub-feature2等。当你在小分支上开发完后就可以提MR将代码合入feature/big-feature分支上。这样由于每个MR都很小，所以code reviewer就可以认真仔细地给你挑毛病了。最后等你把所有的功能都合并到feature/big-feature上后，就可以提一个MR将这个分支的内容合入主分支了。这个时候虽然改动还是很多，不过由于code reviewer已经在之前的阶段看过了你所有的更改，这个时候他要看的东西其实就不多了，对他进行CR也不会有很大的阻碍。

Committer需要注意什么

作为代码的提交者，如果我们希望我们提交的代码可以被code reviewer认真对待并且得到有用的反馈的话，需要注意下面这些方面:

限制每个commit的改动大小和范围

我们CR的时候，最烦的其实就是看到其他人提交了一大堆改动，因为这样我们就需要花很多时间去理解他的逻辑，这也是很多项目很难进行有效CR的主要原因，那就是从开始提交代码的时候就已经错了。作为committer，我们在提交代码的时候需要将改动控制在一个合理的范围，我个人的一个偏好是将改动的文件数控制在5个文件以内，将改动的代码行数控制在150行以内，这样的话reviewer就不需要花费太多时间来帮我们review代码，并且也乐于给我们提供更改意见。除了改动大小的限制，我们同时也要注意commit的范围，确保每一个commit都只做一件事。举个例子: 作为一个前端程序员，你在实现一个导航组件的时候，不要顺带修复一两个bug，或者更改一些配置信息，你应该将你这个commit集中在导航组件的实现上面。换句话来说就是：你要保证你的每一个commit只完成某一个功能或者修复某一个bug，不要将它们"捆绑销售"。

Commit信息要有意义

很多开发者在commit的时候都不会认真写commit message，特别是在外企要求你用英文写的时候：）。工作多年，我见过一些很懒的开发，他们所有的新增功能都叫feature或者是add a new feature，所有修复的bug都叫bugfix或者fix some bug。这些commit message是没有什么意义的，因为它们没有告诉code reviewer这些代码到底开发了什么新功能或者修复了哪个bug。要编写一个好的commit message，首先得给它固定一个格式。现在业界比较推崇和出名的commit message格式是conventional commit[1], conventional commit的主要作用是将我们所有提交进行分类，例如feat代表feature, fix代表bugfix等等。当我们的commit分好类后，除了便于阅读，还有一个好处就是一些自动化生成changelog的工具可以很方便地根据我们某个版本内的所有commit的信息生成这个版本的changelog。除了所有commit message都要遵循某一个固定的格式外，我们还需要在commit message里面写明白我们的代码到底实现了什么功能或者修复了什么bug。如果你在某个commit里面实现了一个用户注册的功能，你的commit信息可以写成feat: implement user signup logic而不是feat: add new feature，如果你修复了登录按钮不可以点击的bug，你的commit信息可以写成fix: login button can not be clicked而不是fix: fix some bug。

Code Review需要看什么东西

那么作为一名合格的code reviewer，我们在帮项目成员进行CR的时候需要看哪些内容呢？根据我个人的经验，我总结了下面这些方面的内容：

正确实现了功能或者修复了bug

作为code reviewer，我们首先要保证的是，committer提交的MR正确地实现了某个功能或者确实修复了某个bug。在我们实际开发的过程中，由于信息传递的不一致或者是开发者理解存在偏差，committer在提交代码的时候可能没有实现完所有的功能或者没有彻底修复QA发现的bug，这个时候我们就可以在做CR的时候指出来防止到QA测试的时候甚至是上线的时候再发现问题。不过要注意的是，这不代表committer不需要自测，相反committer自己在提交代码的时候需要尽最大努力保证自己的代码功能上是没有问题的。

确保编码风格一致

这可能是CR最基本也是最重要的一个目的了。作为一个前端程序员虽然我们可能有一些诸如eslint的工具去帮助我们确保代码格式一致，例如statement后面要不要带分号，缩进是4个空格或者两个空格等。可是由于风格不只是格式，有很多编码风格的问题是不能用eslint这种自动化工具进行约束，而是要靠code reviewer在CR阶段指出来。因此作为code reviewer，我们需要在CR的时候确保合入主分支的代码风格基本保持一致，避免不一样的编码风格降低项目代码的可读性和可维护性。

避免非必要的复杂逻辑设计

因为在同一个项目里面每个人的水平和工作经验都不一样，所以面对同样的问题，有的开发者会用非常优雅和简洁的解决方案去实现，而有的开发者受制于自身的能力，会想出一种非常复杂的解决方案。无必要的复杂的代码逻辑会降低代码的可读性、可维护性和鲁棒性。作为code reviewer，当我们看到过度复杂的代码时一定要指出来，然后尝试和committer一起想出一种更加简单的解决方案(参考开源方案等)。

避免硬编码

硬编码的逻辑和值会降低代码的可读性和可维护性，并且会增加bug出现的概率。因此作为code reviewer，当我们看到一些硬编码的逻辑或者一些诸如magic number等hard-coded values时，可以叫committer将逻辑设计得尽量通用或者将硬编码的值定义为常量值。

提高代码复用率

当我们项目有很多人参与的时候，开发者在实现某个功能时候可能会重复造一些前人造过的轮子，在前端项目里面可以理解成一些公共组件，公共hook或者是帮助方法等。重复的代码一旦合入主分支会降低代码的可维护性并且也会容易引发bug(想一下如果某个需求变了，你要改多个地方就知道了)。因此作为code reviewer，我们在CR阶段的一个重要任务就是识别出代码的重复逻辑。简单来说，重复逻辑包括三种。第一种是committer提交CR的代码和代码仓库现有的代码逻辑发生了重复，这个时候提醒开发者复用之前的逻辑就可以了。第二种是开发者提交的代码里面有重复的逻辑，这个时候需要开发者通过抽取公共函数或者组件的方法来提高自己代码的复用度和内聚度。第三种情况是开发者提交的代码和现有的逻辑有部分重叠，可是现有逻辑不能直接被复用，这种情况需要code reviewer和开发者进行合作，想办法对旧逻辑进行改造从而满足新的需求。举个例子，假如我们现有代码仓库已经实现了用来展示错误消息的Notification组件，committer在新的需求里面提交了一个全新的用于展示警告的Notification组件，这个组件的样式基本和之前的一致只有一些细小的文字颜色的区别，重新写一个的话就意味着当UI设计师更改Notification的样式时，我们需要更改两个地方，这是完全没必要并且低效的。这个时候作为code reviewer，我们要让committer修改之前Notification组件的逻辑让其变得更加通用，然后在新的需求中复用这个组件。当然更改之前组件的逻辑和我们新的需求需要放在两个不同的commit里面，因为我们要限制每个commit的改动大小和范围。

写上必要的代码注释

好的代码注释是用来告诉别人你为什么写这样的代码，而不是告诉别人你的代码做了什么。如果你需要一步步写明白你的代码做了什么的话，首先已经说明你的代码不及格了，因为好的代码风格是会让人通俗易懂的。相反当我们为了克服后端接口的一些bug，或者绕开框架的一些问题而写的一些奇怪逻辑时，我们才需要通过代码注释来告诉别人你写这些代码的目的是什么。除了这些，你也可以使用一些诸如FIXME和TODO的关键词来告诉别人某段代码还有可以提高的空间或者某个功能还没有完全实现。这里值得一说的是，虽然很多开发会用FIXME和TODO等关键词，可是他们喜欢干巴巴地放这么一个关键词在注释里面而没告诉其他开发或者未来的自己什么需要做或者什么需要被提高，这种代码注释同样是没有什么用的。因此作为code reviewer我们不但需要确保committer写了该写的注释，同样也要确保他写的注释是有意义的。

带上必要的自动化测试

CR还有一个重要的作用就是确保committer在提交代码的时候带上必要的自动化测试，例如单元测试和e2e测试等。作为一个前端开发，其实很多人是不写单元测试的，其中一个比较重要的原因是不同于后端接口，前端需要变化太快，刚写完的测试可能下个版本就不能用了，或者是测试用例需要写太多，写测试的时间比写代码的时间还要多。但是我们又不能一点测试都不写，因此测试还是有很多作用的，例如可以帮助我们提前发现问题，好的单元测试还可以当做组件或者函数的文档使用，同时测试也可以帮我们更加高效地进行代码的重构。那么什么代码一定要写测试呢？在我看来，所有的公共逻辑，包括公共组件，公共hook和函数都应该写测试，这是因为公共的东西就意味着使用的人多，如果其中一个人随意改动的话，很容易会造成其他人的逻辑出现问题，因此我们需要通过单元测试来确保我们对于公共逻辑的更改不会引发潜在的bug。另外一个需要写测试的地方是复杂的逻辑。复杂的逻辑写测试的目的是让我们可以尽量模拟不同的输入情况，以提高我们代码的鲁棒性。因此作为一个code reviewer我们在进行CR的时候既要确保committer写了必要的自动化测试，同时也满足一定的测试覆盖率。

总结

在本文中我介绍了一些CR最佳实践的内容，其实按照我个人的经验来说，团队规模小需求不紧的时候，按照上面的要求进行CR还不是一件比较难的事。相反当项目参与的人数变多了，而且版本迭代速度加快后，很多团队开始放松了对CR的要求，可是实际上往往是这个时候我们才更加需要严格的CR来保证代码风格一致以及避免快速迭代导致的代码维护性下降等问题。最后想说得一句话是：做好code review很难很麻烦，不过好的code review实践对项目或者个人的发展都有巨大的作用。

References

[1] conventional commit: https://www.conventionalcommits.org/en/v1.0.0/