代码质量与技术债系列分享之一 - 如何做好 Code Review
TL;DR
参考资料
https://composity.com/post/too-busy-to-improve
https://commadot.com/wtf-per-minute/
https://dl.acm.org/doi/10.1145/3585004#d1e372
https://google.github.io/eng-practices/review/reviewer/standard.html
https://book.douban.com/subject/35513153/
https://zhuanlan.zhihu.com/p/549019453
名词解释
CR: Code Review
CR:代码审查
CL: Stands for "changelist", which means one self-contained change that has been submitted to version control or which is undergoing code review. Other organizations often call this a "change", "patch", or "pull-request".
CL:代表“变更列表”,表示已提交到版本控制或正在进行代码审查的独立更改。其他组织通常将其称为“更改”、“补丁”或“拉取请求”。
LGTM: Means "Looks Good to Me". It is what a code reviewer says when approving a CL.
LGTM:意思是“对我来说看起来不错”。这是代码审查员在批准 CL 时所说的。
CR意义
灵魂拷问:为什么我们接手的每个代码库都如此难以维护?
重要原因之一:Code Review 执行不彻底
万能借口:太忙!
-
疲于应付眼前
-
不可见,意识不到
-
CR 非功能性开发
-
CR 不是当务之急,没有眼前收益
-
危害被低估,忽视“复利”的威力(非线性)
意义
现代代码评审【modern Code Review】,业内认为有效的、基于最佳实践的质量保证工作流,可通过人工审代码降低风险、增强可维护性和提升研发效率,同时可以有效提升个人和团队技术能力。更是一种对代码精益求精、追求极致的态度、是“工匠精神”的一种体现。
CR原则
-
只要CL可以提高整体代码健康状态,就应该倾向于批准合入,即使CL并不完美
-
基于技术事实和数据的沟通
-
基于技术事实和数据否决个人偏好和意见
-
软件设计问题不能简单归结为个人偏好
-
-
解决冲突:不要因为无法达成一致而卡壳
-
善用工具
-
基于Lint、公司代码规范等工具
-
大模型辅助
-
发起CR
发起前的准备
-
推荐自己做一个 checklist
-
把自己当作 Reviewer 来对自己的代码进行 CR
-
预估代码可能出问题的地方
-
进行充分自测,保证代码可运行
-
不要指望别人帮你找出问题
checklist 可参考Code Review 速查手册
利用自动化工具进行前置检查
-
单元测试检查
-
新增单元测试检查
-
方法行数过多
-
圈复杂度过高
-
代码规范检查
-
lint 检查
-
体积监控
建议平均时长不要超过10分钟, 所以 e2e,性能检查等建议不阻塞发起MR流程
合理的规模
https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
-
一次评审200LOC为佳,最多400LOC
-
评审量应低于500LOC/小时
-
一次评审不要超过60分钟
-
采用轻量级评审方式
-
全员参与代码评审
-
每周花费0.5~1天开展CR
-
严格且彻底的评审
如何拆分 CL
https://google.github.io/eng-practices/review/developer/small-cls.html
Commit 描述
Bad Case:
“修复错误”是不充分的 CL 描述。什么 bug?你做了什么来修复它?
其他类似的不良描述包括:
-
逻辑修复
-
添加补丁
-
增加XX规则
-
删除XX逻辑
Good Case:
◆ 摘要:【xx模块】新增xx功能
◆ 背景: 新功能需求,要求xxx, 详见[卡片链接]
◆ 说明: 由于xx,新增xx处理模块…
-
摘要:删除 RPC 服务器消息自由列表的大小限制
-
说明:像 FizzBuzz 这样的服务器有非常大的消息,可以从重用中受益。使自由列表更大,并添加一个 goroutine 随着时间的推移慢慢释放自由列表条目,以便空闲服务器最终释放所有自由列表条目。
必要时,应使用 cz 等工具进行规范。
心态
-
一次 CR 其实是开启的一次“对话”
-
应该期待评论和反馈,并及时进行回复
-
做好心理准备自己的代码可能会有缺陷
-
CR 的目的之一就是发现问题, 所以不要有抵触
CR内容
代码是写给人看的, 不是写给机器看的,只是顺便计算机可以执行而已。------《计算机程序的构造和解释》
应该被 CR 的内容:
自上而下,优先级从高到低:
模块 | 简介 |
---|---|
设计 | 代码是否设计良好并适合您的系统? |
功能 | 代码的行为是否符合作者的预期?代码的行为方式对其用户有好处? |
安全性 | 代码是否安全合规? |
复杂性 | 代码可以更简单吗?当其他开发人员将来遇到此代码时,他们是否能够轻松理解和使用此代码? |
测试 | 代码是否具有正确且设计良好的自动化测试? |
命名 | 开发人员是否为变量、类、方法等选择了明确的名称? |
注释 | 注释是否清晰有用? |
风格 | 代码是否遵循京东代码风格规范? |
文档 | 开发人员是否更新了相关文档? |
https://google.github.io/eng-practices/review/reviewer/looking-for.html
CR流程顺序
https://google.github.io/eng-practices/review/reviewer/navigate
京东实际代码片段评审讲解
设计
应该有合理的职责划分,合理的封装
good case
componentDidMount() {
this.fetchUserInfo();
this.fetchCommonInfo();
this.fetchBankDesc();
}
bad case
componentDidMount() {
const { location, dispatch, accountInfoList } = this.props;
const { token, af } = getLocationParams(location) || {};
dispatch({
type: 'zpmUserCenter/fetchUserInfo',
payload: {
token,
},
}).catch(e => {
const zpmOpenAuthUrlLogin = decodeURIComponent(getCookie('zpmOpenAuthUrlLogin'));
// 如果token过期则跳转回第三方平台
if ([TOKEN_EXPIRED_CODE, '300000'].includes(e.errorCode) && (isSaveUrl(af) || isSaveUrl(zpmOpenAuthUrlLogin))) {
setTimeout(() => {
window.location.href = isSaveUrl(af) ? af : zpmOpenAuthUrlLogin;
}, 2000);
}
});
if (!this.showWhichHeader() && !this.showGatewayHeader()) {
dispatch({
type: 'zpmUserCenter/fetchAccountInfo',
payload: {
token,
},
});
}
this.getBlackList()
}
问题1,fetchUserInfo 未进行封装
问题2,af 命名过于随意
问题3,‘300000’ 魔法字符串
问题4,选择使用 af 或 zpm 这两个URL的逻辑建议封装,避免多次调用 isSaveUrl
优秀代码设计的特质 CLEAN
• Cohesive:内聚的代码更容易理解和查找bug
• Loosely Coupled:松耦合的代码让实体之间的副作用更少,更容易测试、复用、扩展
• Encapsulated:封装良好的代码有助于管理复杂度,也更容易修改
• Assertive:自主的代码其行为和其所依赖的数据放在一起,不与其它代码互相干预(Tell but not Ask)
• Nonredundant:无冗余的代码意味着可以只在一个地方修复bug和进行更改
应避免过度设计
别人在阅读代码时,能清晰辨别我在代码中的设计模式,并且能够随着这个模式继续维护
功能
逻辑正确,逻辑合理,避免晦涩难懂的逻辑
bad case:一段表单代码(原代码过长,仅贴出其中典型的一段)
{ hasQuota ? (
['11', '12'].indexOf(invoiceType) === -1 ? (
<div className="m-b-4 row">
<div className="col-11">
<FormItem
label="基础核验"
>
{basicVerifyStatus ? '已通过' : <div>
未通过
<Tooltip title={basicVerifyMsg} placement="bottom">
<span className='iconfont icon-tishi theme-primary m-l-1 font-14' />
</Tooltip>
</div>}
</FormItem>
</div>
<div className="col-11">
<FormItem
label="剩余额度"
>
{formatAmount(availableLimit)}
</FormItem>
</div>
</div>) :
<div className="m-b-4 row">
<div className="col-11">
<FormItem
label="基础核验"
>
{basicVerifyStatus ? '已通过' : <div>
未通过
<Tooltip title={basicVerifyMsg} placement="bottom">
<span className='iconfont icon-tishi theme-primary m-l-1 font-14' />
</Tooltip>
</div>}
</FormItem>
</div>
</div