Code Review目的与目标
目的：
提升代码可维护性，复用性、鲁棒性(健壮性)、可阅读性
目标：
1.review代码是否符合目录设计约束(每个目录下存在解决具体哪些功能的代码，不可乱写代码)
2.review代码的逻辑缺陷以及边界情况的
3.reivew解决功能的通用性，是否可剥离出通用代码
4.代码实现是否阅读起来无障碍
基本流程
本文后续主要讨论merge前的code reiew
工具选择
- gitlab. 直接可用，无学习成本
什么时候发起/通过/合并代码
发起：提测时（通过自测后）发起，测试和review过程并行
通过：所有comment改完之后（与测试进展无关），reviewer在gitlab上点赞表示通过
合并代码：author自行决定合并代码时机
谁来看
优先服务owner，其次其他熟悉这段业务的人
分级标准
| A | review内容 | 适用场景 |
|---|---|---|
| 不review | - | 半夜hotfix找不到人review |
| 快速review | 只看文件级的整块功能改动 检查merge request checklist 检查新增和改动的配置 | 倒排期需求 强临时性需求 正常需求要求压缩review时间 |
| 标准review | 见“reviewer职责”，业务逻辑正确性审查是optional | 大部分场景 |
常见问题和解决思路
- 业务排的紧，有些甚至是倒排，没时间review
- 敏捷和质量需要做平衡，可以进行一些分级约定
- 需求本身具有很强临时性
- 同上
- 改动量很小，没必要再involve另一个人看
- 既然改动量小，CR势必也能很快看完
- 改动量巨大，没人有空去看
- 控制改动量大的代码在少量场景（新服务，重构）
- 上述分级约定
- 这个模块我自己最资深，谁也看不了
- 人人都会犯错且有知识盲区，不用紧张
Merge request checklist
在repo中新建如下文件：.gitlab/merge_request_templates/standard.md，点merge request时即可出现下拉选项
- [ ] 新增package｜ 描述功能
- [ ] 新增route ｜ 描述功能
- [ ] 新增http接口
- [ ] 新增打点 ｜ 描述功能
- [ ] 新增component ｜ 描述功能
- [ ] 改动原有component | 描述改动点
- [ ] 改动通用函数 ｜ 描述改动点
- [ ] 是否会对其他业务模块产生影响 | 描述影响范围
author职责
description规范
-
Title描述本次
merge request的功能/功能集合 -
Description中，对于多个功能，分多行描述细节 （一般建议一次merge只有一个功能）
-
Description中，对于复杂设计，专门指出或提供链接
-
业务风险点评估
-
基础函数更改时，该函数被调用的所有地方都是可能存在的风险点
- 在基础函数中修改参数时，需要充分考虑所有调用方正常流程是否会被影响
-
在一个流程中插入新的函数，需要评估该函数是否会影响后面的流程
- 函数是否有副作用
- 函数调用失败是处理异常还是抛出异常
-
考虑业务影响范围，本次代码改动有可能会对哪些业务造成影响
-
Review过程注意点
-
发起
review时建议微信通知一下，不然容易被遗漏 -
必须保证自行
review过diff，经验表明：此举能大量减少低级错误 -
author对上线时间负责，有义务催促
reviewer完成review，若reviewer较忙可以换人 -
对于
reviewer的每一个comment，都应该回复：done或做解释 -
merge条件
- 获得赞后
reviewer职责
审查内容
-
业务层面
- 业务划分是否合理。
- 业务是否易理解，逻辑是否正确
-
设计层面
- 组件设计是否合理或者是否有更好的设计
- 目录结构设计是否合理
-
实现层面
- 语言风格规范应由lint保证，看的时候不必刻意检查，但发现问题要提
- 关注是否符合组内开发规范
- 关注项目代码结构是否符合规范
- 关注变量、函数命名是否合理
- 复杂的、特殊的代码段是否有注释
- 是否有性能问题
-
健壮性：错误处理、业务逻辑的边界和基本的安全性
- 边界异常是否完备
- 在
review阶段是否发现明显bug（一般不以发现bug为目的）
Review过程注意点
- 对于复杂业务，先看详细设计文档，找
author做简单交流 - 有争议的点，多走动讨论
- 确保
comments都close了再给赞
