code review的一些实践总结

[cjuexuan]

前言

code review是一个非常重要的一环，我们小组采用merge request模式的code review已经有两年左右了， 这对我们团队的代码质量提升有了比较大的帮助，所以打算这里抛砖引玉分享一下

为什么要做code review

代码质量是一个永恒的话题，不好的代码质量轻则造成维护成本高、后续维护不了不得不重写，重则直接导致同事之间产生矛盾。 对于代码仓库来说，是有破窗效应的。以一幢有少许破窗的建筑为例，如果那些窗不被修理好，可能将会有破坏者破坏更多的窗户。 最终他们甚至会闯入建筑内，对于代码仓库来说，一个人的随意的代码习惯会导致大家都产生这种消极的心态，最终整个代码库的质量就堪忧了。

而code review是保证我们的代码仓库不被腐坏的一个重要手段，单纯从这一点考虑，我们就值得在code review上投入一些时间。 另外我们工作中难免出现岗位调整之后的交接等情况，实施的很好code review可以避免员工调整的单点问题，减少交接的成本。 第三，对于reviewer来说，也不纯粹是时间付出，你也会有收获，这部分的时间投入可以帮你熟悉他的业务，让你开阔眼界，了解更多之前对你未知的领域， 学到一些比较优雅的设计和一些可能之前没接触过的技巧。

我们怎么做code review

每个团队都可能有适合自己的一套code review机制，我这里只是将我们小组的实践经验拿出来分享一下，我们在code review中有大量的讨论， 这部分的内容我觉得非常重要，我们可以在讨论的过程中不断理清思路。

准备工作

1. 制定相同的代码规约标准

这一块我们之前的做法是将类似checkstyle的代码风格约束放在我们的项目构建插件中，在项目创建的过程中 自动会生成我们的scalafix 和scalafmt

2. 让机器人做繁复的约束性检查，比如单元测试和code format

我们工程师应该聚焦在业务逻辑的变更上，而不是帮他找一些低级的问题，所以我们可以很容易的通过gitlab的ci配置测试，在代码产生push之后之后自动触发测试检查，这样在做code review的时候可以减少找低级错误的时间

3. 通过merge request的方式，更正式的进行code review

可能有些团队喜欢用thoughts之类的记录code review的问题，然后让小伙伴整改，这当然可以，但是另一种做法是通过完整的merge request制度，在merge request的过程中我们可以有反复的讨论，也能很容易的diff之前版本的差别， 在不断讨论的过程中，代码向着我们理想的方向不断演进，这个过程就很美好

4. 强调代码风格，避免另类取名等

这一个是强迫症的要求，在service和DAO的取名上，大家都做到一致，比如get获取单个元素，list获取集合元素， 如果仓库里DataAccessObject都叫DAO了，这种情况下自己搞一个叫Dao的类放进去就破坏整体的美感

5. 有条件的话最好超过一人review

我们在设计系统的时候都尽可能的避免单点，在review的时候也一样。一些细微的问题，无论是谁都有可能发现，也有可能疏忽而漏掉，多一个review可以避免主reviewer的压力过大，在主reviewer偶尔精力不集中的时候减少最终code出错的可能性， 同时也能激发大家的讨论热情

如何code review

我们选择了通过merge request的方式，那么怎么让代码更容易被合并呢，可能有的时候做的是一个大功能或者大重构，几十个changes给再有经验的人review也要review半天，这里我们可以做一些事情帮助双方更高效

对于提交者：

1. 大的feature在commit message中附上设计文档的相关链接，描述信息中尽量清晰的加上改动相关的介绍

这一点在开源社区已经被广泛的验证过了，我们注重设计不会使得我们的工作变得低效，反而会让我们返工的次数变少。当然在实施过程中我们尽量避免条条框框的约束，用高效的信息表达我们想要的内容即可，形式可以不限，毕竟不是写八股文

2. 对于reviewer的意见回应

这是起码的尊重，也是能进行充分讨论的基础，如果一个reviewer给你提了几个点，你既不回应也不整改，后续可能大家都没有动力继续去做了，毕竟石沉大海。我们不要因为被别人指出了问题就觉得尴尬或者愤怒，而是应该进行理性的讨论， 理越辩越明，有可能是你写的不好他发现了，也有可能是他没get到你的点，他在这个需求的背景知识可能比较少。

对于阅读代码的人：

1. 对待同事绝不”心慈手软“，以当“喷子”为荣

我觉得code review不是任务，不是作业，要体现它的价值还是要通过高标准控制代码质量，通过讨论发现问题，所以我们不能因为对方催的急，或者自己没有精力就放松标准，我们要能做到合并进去的代码过几天换你维护也是ok的， 你不会自己马上看着不顺眼，准备重构了

2. 聚焦关注点

我的经验是先聚焦接口，自顶向下阅读代码，有些特别细节的地方看不懂不要死磕，可以通过讨论让对方告诉你。 另外关注数据结构的使用，对于并发等结构谨慎怀疑使用者的目的。 对于一些违反设计原则的地方，想一下如果换成自己，能不能用到一些设计模式优雅的解决问题。

3. code review异步化

大家可以每天拿一个自己固定的时间用来做code review，如果组内形成了这种制度，你就不用担心你提交的代码别人review过慢的问题， 非hotfix都是t+1给你解决，提交完代码自己该做什么就做什么，后续也是通过邮件等异步通知，然后讨论即可， 每个人习惯review的时间点都可能不太一样，比如我就喜欢睡觉前花一个多小时review代码～

code review的标准，这里的标准更多是对代码的认同，这边整理了我们平时关注的一些点：

• 命名风格，这一块的话分为强制要求要改正的和建议的，强制要求改正的是那种随手1、2、3的命名（单元测试可以放低要求）， 建议的部分是你觉得原作者的命名不够直观，但还能理解，如果你有更好的，可以作为建议提出
• 接口和跨层调用，这一块的话我们的要求是要严格分层，能抽出接口的要抽出接口，不要出现跨层的调用，比如你的Controller直接调用DAO ,严格的分层会降低review的难度，降低新手上手项目的复杂度，降低在spring容器中循环依赖的风险。比起没有标准还是要更好一点
• 对于过多的if else，case when等明显造成阅读障碍、违反设计原则的部分，通过工厂、策略等设计模式去做优化
• 对于属性、字段等的访问权限严格控制，对于不应该暴露的部分而作者过于公开提出质疑，尤其注意protected方法的暴露问题
• 关注并发结构的使用，对于并发等结构谨慎怀疑使用者的目的
• 关注日志输出这一块，关键的路径有没有日志输出、日志输出是否信息量足够，日志输出是否有性能问题

结束语

code review是一个痛苦但有意义的旅程，小伙伴们还不上车么？由于本人水平有限，难免有一些片面的观点，欢迎大家共同探讨，共同进步

[xiantang]

对于属性、字段等的访问权限严格控制，对于不应该暴露的部分而作者过于公开提出质疑，尤其注意protected方法的暴露问题 我对这个有些🤔️ protected 这边需要注意什么。我这边写代码的时候会用 protected 开出来用来单测

[cjuexuan]

@xiantang 如果纯粹单测还可以，我见过一些过度使用继承关系，滥用protected，以及各种super导致代码难以review，难以静态分析，对于控制权这一块我们需要克制，对于继承的使用也要克制。