关于代码评审的一些思考
words: 3.4k views: time: 11min很多团队都会面临这样的问题，开发人员水平参差不齐，代码写的不够规范，review又觉得费时费力。以下是一些关于代码质量管理与提升的思考，参考总结了网上他人的一些经验。
评审的目的
通常情况下，代码的质量都会随着时间推移而越来越差，尤其是在团队有明确时间限制，让开发觉得不得不采取一些投机取巧的方式才能完成任务的情况下。所以代码评审的主要目的就是保证随着时间的推移，代码能够越来越健康，而不是放任其腐化下去，越来越混乱，以至于最后不得不推倒重来。
评审还有一个目的是希望能提升团队技术氛围，很多开发一味的埋头苦干，很容易忽视团队内的技术分享。再加上团队人员进进出出，有一些正能量的人当然也有负能量的人，这都很正常。但不管怎样我们相信做技术的人都愿意提升自己的技术能力，通过分享，将每个人擅长的领域分享给大家，互相学习来提升凝聚力和团队整体水平。
评审的原则
评审者不必要求开发者打磨好提交中的每个细节，相反应该权衡项目进度和开发给出的意见重要性，适当放宽要求。我们的目标是通过评审能提升整个系统的可维护性、可读性以及可以理解性。我们也不是追求完美，而是持续提高，因为没有完美无缺的代码，只有越来越好的代码。
- 技术和数据高于意见，鼓励质疑，尊重个人偏好；
- 遵守开发规范，保持代码风格，关于风格应该与现有的保持一致，如果以前没有风格，可以就按作者的风格来；
- 软件设计从来不是纯粹的代码风格或是个人偏好问题，它们是基于一些应当被权衡的规则而不仅仅是个人倾向；
评审的关注点
代码评审中比较重要的一点是了解清楚开发者提交变更的意图，把握住变更中的整体设计。代码设计中的一个终极目标是希望能写出高内聚、低耦合的代码。在实际中，我们要求最低要保证代码在结构上层次清晰，逻辑上段落分明，风格应该尽量的简单直白并符合直觉，让别人能比较容易地明白意图。
- 设计良好：首先保证良好的设计，不管是需求设计还是存储设计、接口Api、或是算法流程设计。如果有设计文档，则应该保证文档与代码实现同步；
- 提交说明：描述清楚提交的内容，尽量言简意赅，并保持语义完整。保证提交的变更符合描述，尽量一次变更只做一件事情；
- 文件组织：保证组织结构清晰，按照默认的约定来，比如springboot中的相关配置文件，mvc中的代码层次组织；
- 命名语义：保证语义清晰，包括文件、库表字段、以及类、方法变量等，通过命名能明白其作用，最好能通过命名对文件完成自然的分类；
- 代码风格：保证格式化，段落层次清晰，比如通过一些空行，方法拆解，来体现逻辑的层次感；我们鼓励对代码风格提出改进建议，但要避免吹毛求疵，尤其是通过一些评审工具给出的建议，可以视为锦上添花。
- 职责单一：不管是类、接口、还是方法的定义，它们都有自己对应的角色或者职责，只是维度可能不一样，应该保证它们的分工明确，职责清晰；
- 复杂度：除非一些特殊的处理，比如复杂的算法，否则单个方法的复杂度不应该过高。同样在设计上，如无必要，不增加复杂度。我们鼓励通过设计来提高可扩展性，但不应该为了扩展一些不存在的场景而过度设计，应该反思新增的复杂度是否值得；
- 代码注释：准确而必要的注释(错误的注释不如没有，不必要的注释反而增加维护负担)。通常注释应该解释为什么这么做，而不是做了什么，应该说明代码中没有包含的信息，比如实现的背景。当然也有例外，比如正则表达式或复杂的算法，如果能解释清楚可以帮助别人更好的理解；
- 单元测试：这个我们不做硬性的要求，毕竟写单元测试是一件比较枯燥的事情。但是对于一些关键的api或流程算法，应该进行充分的用例测试；
- 线程安全：如果涉及到可变共享资源的访问，应该格外留意线程安全问题；
评审的步骤
这里主要针对的是线下组织的评审会议，为了避免会议中的讨论走题浪费时间，所以我们应该在会前明确好会议的主题流程
- 评审范围，首先明确改动范围，比如一次变更提交，某个功能或者模块；
- 需求背景，说明改动的意图，说清楚做了什么，期望达到什么目的；
- 设计思路，怎样做的，说明白实现的想法；
- 代码实现，对照设计思路说明代码的实现细节；
- 遗留问题，在设计实现过程中是否还有一些未完成的Todo，或者一些妥协；
- 评审意见，可以在git上以Issues的方式提出意见，由评审人落实修改；
关于评审的过程实践
- 代码提交
作为一个开发人员，写清楚提交备注是一个必要的职业素养，至于一些不按照规范来写的开发可以对他们提出要求。为了将提交记录与具体的开发任务关联起来，我们可以采用 git + jira 的方式，让开发人员在每次提交的comment中增加jira编号。但这只是一个规范，如何将规范真正落地，并避免开发人员在comment中忘记，可以采用一些工具帮助开发人员在提交时进行检查。git的hook机制在代码提交前后提供了一些钩子，可以用来控制提交允许或拒绝，比如说pre-commit或commit-msg。
- 代码检查
对于编码规范，我们也采用了sonar去扫描问题，但这个方式的缺点是太过滞后，需要质量人员去跟进推动并且效果也不是很好。因为大多开发都有一种习惯，如果代码写完上线之后没有问题的话，他们很少会主动去优化代码，即使你告诉他扫描有问题他也会有各种理由推脱，也许你可以通过管理手段强制他们修改，但这些都是比较滞后的方式。可以考虑采用 git + checkstyle或fingbug等工具插件，来前置检查点帮助开发人员在开发过程中就发现问题并进行解决。在Maven工程中，也有代码检查相关的plugin，比如checkstyle、pmd或findbugs。
- 代码分支
团队中每个人或多或少对git的管理与使用理解有些不一致，可能造成分支、版本管理的混乱，这样在代码合并时可能产生很多不必要的冲突。我们可以制定一套规范性的东西，来进行进行分支和版本的管理，让开发在可控的范围内自由发挥。比如我们可以制定如下分支规范：
master分支：只有一个，并且控制分支的push与merge权限；
dev-{taskName}分支：开发分支，从master创建，可以结合jira的sprint，通常以是一个发布周期或者一个迭代来命名；
feat-{userName}分支：开发人员分支，从开发分支dev-{taskName}创建，这个分支的声明周期通常很短，在功能开发完成通过Merge Request发起合并，并进行code review之后合并，最后删除分支；
- 代码评审
比较常见的方式是团队内定期组织全体开发人员进行集中式的code review，但长期这样会有点费时费力效率不高。可以参考开源团队，利用工具在线来进行review。比如说github有pull request，gitlab有merge request，可以在代码合并的节点上进行review，这样的好处是比较开放，而且可以留下review记录，互相的想法和沟通建议都可以很好的留存下来并通过UI的方式友好的展示出来。
Java代码示例
- 代码组织
对于一般的springboot应用，我们可以约定下面这样的工程结构。对于代码分层，可以沿用传统mvc的controller/service/mapper/entity，当然如果想避免service层之间相互引用，导致循环依赖，可以在service与mapper之间再加一层manage，来定义service层中的公用处理。另外考虑到可能还有一些其他的package定义，比如util或者configuration之类，所以在mvc之上加一层api，并与其他的package保持同级，这样包的结构看起来更清晰一点，而且也对应我们的接口定义格式：http://{ip}:{port}/{cotext-path}/api/v1/xxx
1 | {项目}-{服务xxx} |
有的项目中，可能多个服务之间存在相互调用，依赖彼此提供的接口。那么这些接口应该由提供方来统一定义，然后由其他调用方来依赖和调用，而不是放在各个调用方中重复定义，容易造成混乱。针对这种场景，我们可以将代码拆成不同的module，可以参考下面结构：
1 | {项目x}-{服务xxx} |
- 关于常量类和工具类
很多开发同学喜欢定义Constants常量类以及Util工具类，但我们觉得应该尽量避免定义这样的额外类型。因为通常每个常量都应该有它对应的数据模型定义，比如下面这样：
1 | public class Constants { |
这里定义了两组常量，人的性别和岗位的状态。但如果还有一个其他的数据也有状态这个属性，那么是让它复用这里岗位状态呢，还是通过前缀区分再加一组状态常量呢。虽然都可行，但是肯定不如将这个状态常量直接放到对应数据类型中来得更加直白。而且Constants没有具体的业务含义，当定义的常量值变多之后，容易变得混乱，如果项目中发生人员变动，后面接手的开发同学很容易继续向其中添加更多的常量，导致重复和更多混乱。
1 | public class SysPost { |
同样的对于一个方法，如果并不具备独立出来进行复用的价值，那么也不应该定义成Util中静态方法，既然是方法一般也有它对应的归属类型。对于那些确实可以定义成Util的方法，我们可以建议先检查一下这样的方法是否已经有人提供了，比如Apache的commons库，或者Hutool等第三方库。