25.CodeReview实践总结

目录介绍

• 01.CR机制的介绍
  • 1.1 CR机制的背景
  • 1.2 CR机制的目的
  • 1.3 代码Merge介绍
  • 1.4 Review串讲
• 02.Git配置CR实践
  • 2.1 Git配置CR
  • 2.2 CR提交实践
  • 2.3 CR如何打退
  • 2.4 CR通过说明
  • 2.5 高效CR实践
• 03.CodeReview清单
  • 3.1 普通常规项
  • 3.2 代码安全项
  • 3.3 无效沉余项
  • 3.4 文档注释项
  • 3.5 一些其他细节
• 04.CodeReview实践
  • 4.1 CodeReview方式
  • 4.2 CodeReview输出
  • 4.3 CR注意事项说明
  • 4.4 CR标准模版说明
• 05.代码如何自查
  • 5.1 对比新老代码
  • 5.2 Studio工具检查

01.CR机制的介绍

1.1 CR机制的背景

• 修改代码导致不可控
  • 修改代码所带来的bug比新开发功能的bug并不会少，因为测试人员并不完全知道你所修改的部分到底影响到了哪些子功能或者哪些其它的模块。
• 大需求设计存在不合理
  • 针对比较大的业务需求，尤其前期时间比较紧，开发业务想着先是实现功能。而忽略了整体架构的设计，分层，导致业务后期迭代越来越难。
• CodeReview的背景
  • 通过CR对每一个小的改动追根究底的询问和跟踪，能够发现测试无法发现的一些隐藏的诡异的问题，大家相互CR还能快速熟悉业务。
• Google的CR机制说明
  • 在Google，任何产品，任何工程的代码，在被进行严格或者明确的审查（code review）之前，是不允许提交的。
  • CR是严格的软件开发中一个通用的规则。不仅仅是产品代码这样——而应该是一切。它不会占用特别多的工作时间，但是会带来巨大的不同。

1.2 CR机制的目的

• 通过code Review的代码可以上升到更高的层次。
  • 1、提升开发人员代码编写质量
  • 2、现有版本的优化
  • 3、开发人员间代码和业务互相熟悉

1.3 代码Merge介绍

• Gitlab提供了两种代码merge机制：
  • 第一种：在本地将源分支(Source branch)代码合并到目标分支（Target branch），然后Push到目标分支（Target branch）
  • 第二种：将源分支(Source branch)Push到远端，然后在GitLab指定目标分支（Target branch）发起Merge Request，对目标分支（Target branch）拥有merge权限的用户执行Merge操作，完成合并。
• 那种方式提交才能CR操作
  • 第二种适合CodeReview，发起MR后，由有Merge权限用户做CodeReview，通过后执行merge操作。
• CommitMessage规范如下
  • 应该尽量清晰明了，说明本次提交的目的。说明commit的类别，常用的标识如下：

  feat：新功能
  fix：修补bug
  docs：文档
  style： 格式（不影响代码运行的变动,空格,格式化,等等）
  refactor：重构（即不是新增功能，也不是修改bug的代码变动)
  perf: 性能 (提高代码性能的改变)
  test：增加测试或者修改测试
  build: 影响构建系统或外部依赖项的更改(maven,gradle,npm 等等)
  ci: 对CI配置文件和脚本的更改
  chore：对非 src 和 test 目录的修改
  revert: Revert a commit

1.4 Review串讲

• 代码CR的粒度尽可能小
  • 尽可能保证MR颗粒度适中，应避免颗粒度过小导致过多MR，也应避免颗粒度过大，给review带来复杂度不易发觉问题。
• 针对比较大而且独立模块的开发，负责人可以在小组串讲代码。
  • 利用大约40分钟时间串讲代码架构设计，代码编码，代码核心流程，代码遇到的坑，代码的亮点等。

02.Git配置CR实践

2.1 Git配置CR

2.2 CR提交实践

• CR核心流程
  • 1.创建分支。包括：总开发分支（e.g: feature/app_3.7.0）和各老师自己开发分支（e.g: feature/app_3.7.0_yc）
  • 2.Push修改并提交MR。各老师开发完成后push到远端，通过gitlab页面提交MR，指定Reviewer 和 Assignee（一般指定Reviewer即可）。
  • 3.Code Review。Reviewer 进行code review, 若没有发现问题，可以直接点击merge;如果发现问题，可以发起讨论或者直接关闭MR。
• CR具体操作流程
  • 本部分主要描述：代码提交，发起MR，进行code review，提交merge。
  • 第一步：代码提交(这里是自己的那个分支)，提交后然后查看commit记录，切到开发分支。
  • 第二步：发起MR(Merge requests)，注意要选好源分支和目标分支。
  • 第三步：CodeReview，接口人查阅代码。
  • 第四个：Merge代码，如果是通过则直接merge，如果是不通过则要指出详细的原因。

2.3 CR打退说明

• CR不通过需要说明
  • 代码审核者在此过程中可以随时提出自己的疑问，同时积极发现隐藏的bug；对这些bug记录在案。
  • 代码审核者把Code Review中发现的有价值的问题更新到"代码审核规范"的文档中。
• CR打退后修改代码
  • 代码编写者根据“代码审核描述”给出的修改意见，修改好代码，有不清楚的地方可积极向代码审核者提出。

2.4 CR通过说明

03.CodeReview清单

3.0 CR审查重点

• 事前准备阶段
  • 在准备CR代码对象时，要注意代码的数量，如果代码量比较大，要对代码进行必要的分解，确定其中的关键代码，对关键代码进行CR，可以达到举一反三的目的。
  • 对代码的审查内容很多，如代码的编写是否规范、技术处理规范(异常处理、日志处理、代码组织结构等)、业务实现等。设定本次CR活动内容界限，确定审查重点。
  • 在CR前设计确定评审规范和标准是必要，通过规范和标准我们在审查过程中可以有据可依，有理可循，而且还可以做到标准统一。
• 事中实施阶段
  • 对于CR过程发现的问题，必须清晰准确的记录，可以使用问题点记录单，明确记录的项目和内容。
  • CR过程中，要采用代码作者讲解和审查者提问方式。审查者不能只在发现问题时提问，同时也要根据本次审查的内容要求代码作者对某个特定问题的讲解。
  • 实施审查时，要营造一个讨论问题、解决问题的氛围，不能把审查会搞成批判会，这样会影响相关人员的积极性。
• 事后跟踪解决
  • CR结束后，对发现的问题，首先需要确定以下内容。1.问题点的难易程度以及影响的范围；2.解决问题的责任者和问题点修正结果的确认者；3.解决问题点的时限。
  • 对于修正问题责任者，在问题点的修正过程中，要三方面内容的记录。1.问题点的原因；2.解决问题点的对策；3.修正的内容。

3.1 普通常规项

• 关于功能上检查
  • 有没有实现预期的功能，逻辑是否正确？
  • 循环是否设置了长度和正确的终止条件？
  • 是否有可以被库函数替代的代码？
  • 是否有可以删除的日志或调试代码？尤其是一些debug代码不能带到线上的？
  • 使用 ?. 替代 !!，在 Kotlin 中尽量少使用 !!，建议可以用 ?. 避免空指针异常
  • 对于 if-else 嵌套条件， 需要仔细检查是否符合业务逻辑； 如果嵌套太深，是否可以使用另一种方式“解结”
  • 缺乏日志并不会影响业务功能，但出现问题排查时，就会非常不方便。关键节点添加日志。
  • 对错误码进行可控的管理和遵循规范使用。可以使用公共文档维护。
  • 代码中是否存在任何没有定义或没有引用到的变量、常数或数据类型。
  • 方法的边界条件有没有考虑等，尤其是有多个分支逻辑时需要注意。
• 关于代码规范检查
  • 代码符合你所遵循的编程规范么？这通常包括大括号的位置，变量名和函数名，行的长度，缩进，格式，括号，空白行和注释等。
  • 是否存在多余的或是重复的代码？重复的代码是否有抽取？
  • 代码是否尽可能的模块化了？比如针对比较独立的功能，是否抽取封装了？
  • 是否有可以被替换的全局变量？是否有被注释掉的代码？针对常量，是否有注释？
  • 魔法数。尽量避免使用魔法数字，应代之有名字的常量或枚举
  • 命名不贴切不会影响功能实现，却会误导理解或增加理解难度。命名一定要规范？

3.2 代码安全项

• 关于代码逻辑安全检查
  • 所有的数据输入是否都进行了检查（检测正确的类型，长度，格式和范围）并且进行了编码？
  • 无效的参数值是否能够处理？
  • 针对if else代码，最后是否处理else分支逻辑？针对递归是否有出口？
  • 输出的值是否进行了检查并且编码？
  • 如果API参数有多个，而且相邻参数的类型相同，那么要特别留意是否参数顺序是正确的？
  • 类似 if ((!A || !B) && C || (D && E)) 的多重条件要仔细推敲。方法： 最好拆分成多个有含义变量。
• 关于代码异常检查
  • 在哪里使用了第三方工具，返回的错误是否被捕获？
  • 针对try-catch操作，为何要这么做？对业务或者功能有何影响？
  • 代码中存在大量的 warning 警告能否清除？
  • 异常逻辑建议增加日志，方便后续定位问题，或者对异常逻辑进行上报，观察问题的数量级

3.3 无效沉余项

• 关于注释的代码
  • 代码注释了后，是否需要删除掉？废弃代码建议直接删除掉，后续想找回，回溯 Git 的 history 即可

3.4 文档注释项

• 三方库需要有文档
  • 第三方库的使用和函数是否有文档？如果是沉淀lib，则需要完善demo和api调用文档！
• 关于代码注释检查
  • 是否有注释，并且描述了代码的意图？所有的函数都有注释吗？
  • 对重要和关键点的代码缺乏必要的注释，使用到的重要算法缺乏必要的引用出处，对特别的处理缺乏必要的说明。
  • 对非常规行为和边界情况处理是否有描述？
  • 数据结构和计量单位是否进行了解释？
  • 是否有未完成的代码？如果是的话，是不是应该移除，或者用合适的标记进行标记比如‘TODO’？

3.5 一些其他细节

• 代码规范方面的细节
  • 未使用 import；
  • 未使用的资源，例如 drawables, strings, colors……
  • 注释代码；
  • 未格式化的代码；比如代码之间没有适当的空格，建议随手格式化一下
• 代码语法方面的细节
  • 变量名、方法名、文件名……查看命名是否符合规范，是驼峰，还是下划线

04.CodeReview实践

4.1 CodeReview方式

• 第一点：抓重点，比如设计、架构、可读、健壮。
• 第二点：所有人参与codeReview
  • 简单点：技术经理可以从这周提交的代码中找出典型的代码片段，拿出来进行讲解和讨论；
  • 完善点：每个开发者互相codeReview，找出有问题的代码片段，拿来给大家进行讨论。
  • 灵活点：遇到问题就可以跟开发人员面对面沟通。

4.2 CodeReview输出

• 输出成实例规范
  • CodeReview后，发现开发人员比较好的实现方式和思路，可以总结成开发规范，最好能加上实例，这样方便记录和开发人员的规范统一，做到代码的实现看上去是一个人写的。

4.3 CR注意事项说明

• 经常进行CodeReview
  • 程序员代码写得时候越长，程序员就会在代码中加入越来越多的个人的东西。经常CR，其实也是经常跟人讨论技术，保持进步。
• CodeReview不要太正式
  • Review只不过是一种形式，而通过相互的讨论得到了有意义和有建设性的建议和意见，那才是最实在的。
• 让不同人Review代码
  • 不要总是只找一个人来Review你的代码，不同的人有不同的思考方式，有不同的见解，所以，不同的人可以全面的从各个方面评论你的代码。
• 保持积极正面的态度
  • 需要能够虚心接受别人的建议，因为别人的建议是为了让你做得更好，评审者也需要以一种积极的正面的态度向作者提意见。

4.4 CR标准模版说明

05.代码如何自查

5.1 对比新老代码

5.2 Studio工具检查

• 执行Android studio –> Analyze –> Inspect code操作之后，所有的lint警告列表就会出来。于是得到六大类Android Lint
  • Correctness 正确性
  • Internationalization 国际化，如字符缺少翻译等问题。
  • Performance 性能，例如在 onMeasure、onDraw 中执行 new，内存泄露，产生了冗余的资源，xml 结构冗余等。
  • Security 安全性，例如没有使用 HTTPS 连接 Gradle，AndroidManifest 中的权限问题等。
  • Usability 易用性，例如缺少某些倍数的切图，重复图标等。
  • Accessibility 无障碍例如 ImageView 缺少contentDescription 描述，String 编码字符串等问题。