因为最近在工作中参与了团队的一些CodeReview（CR）规范的制定，所以在这里分享一些我们积累的CR最佳实践。本文将包含以下内容：为什么需要CodeReview什么时候做CodeReviewCommitter需要注意CodeReviewer需要看什么为什么需要CodeReview对于参与人数大于等于两人的项目，CR是必不可少的一项活动。在我看来，它有以下优点：可以提高committer对自己代码的要求。根据我以往的经验，有的人在review的时候和不review的时候，代码风格是完全不一样的。如果一个人写的代码总是直接push或者merge到主分支而不被别人阅读，这个人在写代码的时候就不会有任何顾忌，想写什么就写什么，反正没人看。但是当他的代码需要同行review的时候，他的心态就完全不一样了。这个时候，他会想办法简化代码，去掉一些硬编码的逻辑去追求一些最佳实践，各种TODO和FIXME也会写清楚。因为如果他什么都不做，作为一个合格的codereviewer，我们是绝对不会把他的代码合并到主分支的。如果长期如此，committer的代码水平会逐渐提高。群内知识共享俗话说：你一个苹果，我一个苹果，大家交流，每人依然只有一个苹果；你有一个想法，我有一个想法，我们交流，每个人都有两个想法。这句话同样适用于我们的软件开发。作为程序员，当我们面对同样的问题时，我们可能会有不同的解决方案。如果没有沟通，大家可能永远只知道一种解决方案，无法取得进展。那么程序员如何相互学习呢？最简单的方法就是看别人的源码，而CR是阅读别人源码最好的过程。因为我们在给一个committer做review的时候，他提交的代码只专注于解决一个问题，所以代码量不会太大，我们也可以更清楚的了解到他是如何解决某个问题的。如果committer恰好用了我们以前不知道的非常巧妙的方法来解决问题，我们就可以学到新的知识。保持项目代码风格的一致性同一个项目中不同的程序员背景不同（可能来自不同的项目和公司），解决问题的方法和思路也不同。作为一个前端程序员，我见过redux-saga和redux-thunk都在同一个代码仓库中作为异步中间件使用。如果作为代码审查者，我们不对committer的代码进行限制，那么项目的代码风格就会出现两极分化，这无疑会增加项目的维护成本和新开发者的学习成本。提前发现代码问题一些经验较少的开发者在写代码的时候可能没有全面考虑问题，导致一些边缘情况（edgecases）没有被考虑到。这时候如果codereviewer是工作经验比较多的同学，可以帮助committer提前发现问题，避免产品上线后浪费时间去定位问题。值得注意的是，即使代码审查者不是很资深的开发人员，作为代码的局外人，他在编写代码时，往往也能考虑到当事人（committer）无法考虑的情况。何时进行CodeReviewCR必须发生在单元测试、lint检查等各种CI自动化检查通过之后，代码正式合并到主分支之前。这里涉及到的一个问题是，如果我们开发的功能涉及到很多改动，我们开发完之后是一起展示给reviewer看，还是拆分成小的MR（MergeRequest）给reviewer看？我个人倾向于后者。因为如果你提了很多代码，codereviewer需要花很多时间阅读你的代码，了解各个模块是如何协作的，问你问题之后，你可能需要改很多，这就导致了整个CR周期变得很长，实际上会打击codereviewer给你找茬的积极性。在这种情况下，一些不耐烦的代码审查者可能会给你一个简单的LGTM然后将你的代码合并到主分支中，这样的CR是没有意义的。因此，更好的做法是在开发大功能时将代码拆分成小模块，将每个小模块合并到主分支中，直到所有功能都合并到主分支中。但是如果开发者不想将他未完成的功能模块合并到主分支中怎么办？这时候可以使用一种叫做stackedCR的模式：先从主分支切出一个feature/big-feature分支，后面开发这个big-feature的时候，总是从这个feature/big-feature切出branch各种features的小分支，比如feature/big-feature-sub-feature1,feature/big-feature-sub-feature2等，当你在小分支上开发完成后，可以提交MR合并代码到功能/大功能分支。这样，由于每个MR都非常小，所以codereviewer可以很仔细的给你挑错。最后，当你将所有的功能合并到feature/big-feature之后，你可以提交一个MR，将这个分支的内容合并到主分支中。虽然这个时候还有很多变化，但是由于codereviewer已经看到了你上一阶段所有的变化，所以这个时候他需要看的东西其实并不多，对他的CR也不会有太大的阻碍。Committer需要注意什么作为代码提交者，如果我们希望我们提交的代码得到代码审查者的重视并得到有用的反馈，我们需要注意以下几个方面：限制每次提交的更改大小和范围我们CR的时候，最讨厌的就是看到别人提交一大堆修改，因为我们需要花很多时间去理解他的逻辑。这也是很多项目难以进行有效CR的主要原因，即从提交代码开始的时间就已经不对了。作为committer，我们在提交代码的时候需要把改动控制在一个合理的范围内。我个人的一个喜好是把改动的文件数控制在5个文件以内，改动的代码行数控制在150行以内。如果是这样，reviewer就不需要花太多时间来帮我们review代码，也很乐意为我们提供修改建议。除了改变大小限制，我们还要注意commit的范围，保证每次commit只做一件事。举个例子：作为一个前端程序员，当你实现一个导航组件的时候，不要修复一两个bug或者更改一些配置信息，你应该把你的commit集中在导航组件的实现上。换句话说：你要保证你的每一个commit只完成某个功能或者修复某个bug，不要“捆绑”。Commitmessage应该是有意义的很多开发者在commit的时候并没有仔细的写commitmessage，尤其是当国外公司要求你用英文写的时候:)。工作多年，见过一些非常懒惰的开发者。他们所有的新功能都称为功能或添加新功能，他们修复的所有错误都称为错误修复或修复某些错误。这些提交消息是没有意义的，因为它们没有告诉代码审查者代码开发了哪些新功能或修复了哪些错误。要想写好commitmessage，首先要给它一个固定的格式。业界比较推崇和知名的commitmessage格式是conventionalcommit。conventionalcommit的主要作用是对我们所有的提交进行分类，比如feat代表feature，fix代表bugfix等等。当我们的commit被归类后，除了便于阅读之外，还有一个好处就是一些自动生成changelogs的工具可以很方便的根据某个版本所有commit的信息生成这个版本的changelog。除了所有的commitmessage必须遵循一定的固定格式之外，我们还需要在commitmessage中写清楚我们的代码实现了哪些功能或者修复了哪些bug。如果你在提交中实现用户注册功能，你的提交信息可以写成feat:implementusersignuplogic而不是feat:addnewfeature。如果你修复了登录按钮无法点击的bug，你的commit消息可以写成fix:loginbuttoncannotbeclicked而不是fix:fixsomebug。代码审查员需要看什么？作为一个合格的codereviewer，我们在帮助项目成员进行CR时需要看什么？根据我个人的经验，我总结了以下几个方面：正确实现的功能或修复的错误。一个错误。在我们实际的开发过程中，由于信息传递的不一致或者开发者理解上的偏差，可能导致committer在提交代码时没有实现所有的功能或者完全修复QA发现的bug。这个时候我们可以在CR的时候做Pointitout，防止在QA测试的时候甚至上线的时候发现问题。但是需要注意的是，这并不代表committer不需要自测。相反，committer在提交代码的时候需要尽最大努力保证自己代码的功能没有问题。确保一致的编码风格可能是CR最基本和最重要的目的。作为前端程序员，虽然我们可能有eslint之类的一些工具帮助我们保证代码格式的一致性，比如语句后是否要有分号，缩进是4个空格还是2个空格等等。但是因为风格不仅仅是格式，所以有很多编码风格的问题是无法通过eslint等自动化工具约束的，需要在CR阶段由codereviewer指出。因此，作为代码审查者，我们需要保证CR时合并到主分支的代码风格基本一致，避免不同的编码风格降低项目代码的可读性和可维护性。避免不必要的复杂逻辑设计，因为在同一个项目中每个人的水平和工作经验都不一样，所以面对同样的问题，有的开发者会用非常优雅简洁的方案来实现，而有的开发者受限于自身能力，会想出有一个非常复杂的解决方案。不必要的复杂代码逻辑会降低代码的可读性、可维护性和健壮性。作为codereviewer，当我们看到过于复杂的代码时，一定要指出来，然后和committer一起想出更简单的解决方案（参考开源方案等）。避免硬编码降低代码可读性和可维护性，增加错误机会的硬编码逻辑和值。因此，作为codereviewer，当我们看到一些硬编码的逻辑或者一些硬编码的值比如magicnumber时，我们可以要求committer将逻辑设计得尽可能通用或者将硬编码的值定义为常数值。提高代码复用率当我们的项目参与人数较多时，开发者在实现某个功能时可能会重复创建之前的一些轮子。在前端项目中，可以理解为一些公共组件，公共hooks或者help方法等，重复的代码一旦合并到主分支中，会降低代码的可维护性，容易造成bug（想想关于如果某个需求发生变化，你必须改变多个地方才能知道）。因此，作为代码审查者，我们在CR阶段的一项重要工作就是识别代码的重复逻辑。简单来说，重复逻辑分为三种。首先是committer提交的CR代码与代码仓库已有的代码逻辑重复。这个时候提醒开发者复用之前的逻辑就可以了。二是开发者提交的代码中存在重复逻辑。这时候，开发者就需要抽取公共的功能或者组件来提高自己代码的复用性和内聚性。第三种情况是开发者提交的代码与已有逻辑有部分重叠，但无法直接复用已有逻辑。在这种情况下，codereviewer和developer需要合作，想办法改造旧的逻辑来满足新的需求。例如，如果我们现有的代码仓库已经实现了显示错误信息的Notification组件，committer在新的需求中提交一个新的显示警告的Notification组件。该组件的样式与上一个基本相同。只有一些小的文本颜色差异，重写一个意味着当UI设计者改变Notification样式时，我们需要改变两个地方，这是完全没有必要和低效的。这个时候，作为codereviewer，我们需要让committer修改之前Notification组件的逻辑，让它更通用，然后在新的需求中复用这个组件。当然，更改之前组件的逻辑和我们的新需求需要放在两个不同的commit中，因为我们需要限制每次commit中更改的大小和范围。写必要的代码注释好的代码注释是用来告诉别人你为什么写这样的代码，而不是告诉别人你的代码做了什么。如果你需要一步步写明白你的代码做了什么，首先，它已经说明你的代码失败了，因为好的代码风格是容易理解的。相反，当我们为了克服后端接口的一些bug，或者绕过框架的一些问题，写了一些奇怪的逻辑时，我们就需要通过代码注释来告诉别人写这些代码的目的是什么。除了这些，你还可以通过一些关键字比如FIXME、TODO来告诉别人某段代码还有改进的空间或者某个功能还没有完全实现。这里值得一提的是，虽然很多开发者会使用FIXME、TODO等关键字，但他们喜欢在评论里放这样的关键字，而不告诉其他开发者或者未来的自己需要做什么，需要改进什么，这种代码注释也没有用。因此，作为代码审查者，我们不仅要保证committer写了该写的注释，还要保证他写的注释有意义。带来必要的自动化测试CR的另一个重要作用是确保committer在提交代码时带来必要的自动化测试，比如单元测试和e2e测试。作为前端开发者，很多人是不会写单元测试的。更重要的原因之一是，与后端界面不同，前端需要更改得太快。刚写的测试可能下个版本就没有了，或者测试用例太多要写，写测试的时间比写代码的时间还多。但是我们也不能完全不写测试，所以测试还是有很多作用的，比如帮助我们提前发现问题，好的单元测试也可以作为组件或者功能的文档，测试也可以帮助我们更有效率。代码重构。那么必须要写什么代码来测试呢？在我看来，应该测试所有公共逻辑，包括公共组件、公共钩子和函数。这是因为公共的东西意味着很多人使用它们。如果随意改动其中一个，很容易导致其他人的逻辑出现问题，所以我们需要通过单元测试来保证我们对公共逻辑的改动不会造成潜在的bug。另一个需要编写测试的地方是复杂的逻辑。编写复杂逻辑测试的目的是让我们尽可能地模拟不同的输入情况，以提高我们代码的健壮性。因此，作为代码审查者，我们在进行CR时，不仅要保证committer编写了必要的自动化测试，还要满足一定的测试覆盖率。总结在本文中，我介绍了一些CR最佳实践。其实根据我个人的经验，在团队规模小，要求不紧的情况下，按照上面的要求进行CR并不难。相反，当项目参与人数增多，版本迭代速度加快后，很多团队开始放宽对CR的要求，但实际上往往是在这个时候，我们需要严格的CR来保证一致性代码风格并避免快速迭代。由此导致的代码可维护性下降等问题。最后我想说的是：做好codereview是一件困难且麻烦的事情，但是良好的codereview实践对项目或个人的发展有着巨大的作用。欢迎关注公众号攻击大葱个人技术动态，一起学习成长
