代码质量与技术债系列分享之一 - 如何做好 Code Review

TL;DR

Code Review 速查手册

参考资料

名词解释

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 ? (

关键问题：连续三元判断 + 嵌套三元判断

其他问题：

魔法字符串， 且重复出现

['11', '12'].indexOf(invoiceType) === -1

无意义的空行，严重影响代码阅读

FormItem 重复过多

Reviewer 建议：

对重复代码，梳理内容，进行合理命名

const isNotOnlineInvoice = ['11', '12'].indexOf(invoiceType) === -1;

每个 FormItem 也进行命名，三元逻辑梳理，重构

安全性

代码中应注意，不要存储敏感内容

// 微信服务号 生产配置中复写const WX_APP_ID = 'xxxxxxxxxx';const WX_APP_SECRET = 'xxxxxxxxxxxxxxxxxxxxx';// 票将军网站应用配置 （测试环境）const PJJ_APP_ID = 'xxxxxxxxxx';const PJJ_APP_SECRET = 'xxxxxxxxxxxxxxxxxxxxx';

一致性

代码一致性：

函数名和实现一致

注释和代码一致

命名方式一致

异步写法一致（promise， async await，callback混用）

抽象层级一致

不建议混用 import 和 require

注释与代码不一致

getCheckboxProps: record => ({ disabled: !record.basicVerifyStatus || (hasQuota && record.availableLimit <= 0), // 状态为验证成功的时候才可选择使用})

命名不一致

this.state = { isOffline: false, shouldShowFollowLink: false, shouldShowToast: false, ifReceiveNotify: false, bShowAllDocsRedPoint: false, isNewPushNotify: false,}

没事别写注释

good case：

为什么接下来的代码要这么做

bad case：

接下来的代码做了什么

复杂度

优先使用标准库中的能力

封装细节

写的代码越简单, bug越少

尽量遵守单一职责原则

DRY——Don’t Repeat Yourself

无意义的函数封装

// 根据admitStatus判断按钮试算前置灰状态export const isDisabledByAdmitStatus = discountListItem => { if (!discountListItem?.bankInfo?.admitStatus) { return true } else { return false }}

建议使用moment、dayjs等标准时间库处理时间：

// 本季度开始时间、结束时间，返回毫秒值export const getQuarterStartAndEndTime = ({ time = null, isTimestamp = true, split = '/', startDateTime = ' 00:00:00', endDateTime = ' 23:59:59',} = {}) => { let date = checkDate(time) ? new Date(time) : new Date let year = date.getFullYear let month = date.getMonth + 1 let startTime = null let endTime = null if (month <= 3) { startTime = year + split + '01' + split + '01' + startDateTime endTime = year + split + '03' + split + '31' + endDateTime } else if (month > 3 && month <= 6) { startTime = year + split + '04' + split + '01' + startDateTime endTime = year + split + '06' + split + '30' + endDateTime } else if (month > 6 && month <= 9) { startTime = year + split + '07' + split + '01' + startDateTime endTime = year + split + '09' + split + '30' + endDateTime } else { startTime = year + split + '10' + split + '01' + startDateTime endTime = year + split + '12' + split + '31' + endDateTime } // 本季度开始时间 startTime = isTimestamp ? getTimestamp(startTime) : startTime // 本季度结束时间 endTime = isTimestamp ? getTimestamp(endTime) : endTime return { startTime, endTime, }}

DRY——Don’t Repeat Yourself

下面三个方法中重复逻辑非常多，应该进行合理的封装，降低复杂性。另一个比较常见的问题，console.log 这种调试代码不应该被合入主干。

handleMergedInvoice = selectedRows => { const { invoiceList } = this.state; const { limitNum } = this.props; const newInvoiceList = []; for (const item of selectedRows) { if (newInvoiceList.length && newInvoiceList.length >= limitNum) { Message.error(`上传失败，发票数量不可超过${limitNum}张`); return; } item.invoiceUrl = item.invoiceUrl || item.dataUrl || ""; delete item.dataUrl; item.invoiceDate = item.invoiceDate ? moment(item.invoiceDate).format("YYYY-MM-DD") : ""; newInvoiceList.push({ ...item, id: getIncrementCid }); this.setState({ invoiceList: newInvoiceList }, => { const { invoiceList: list, errIndexList } = this.state; if (list.length) { this.verifyInvoiceList(list); } this.invoiceListReduce; }); }};updateInvoice = ({ ...data }, i) => { const { invoiceList } = this.state; const oldInvoice = invoiceList[i]; data.invoiceUrl = data.invoiceUrl || data.dataUrl || ""; delete data.dataUrl; data.invoiceDate = data.invoiceDate ? moment(data.invoiceDate).format("YYYY-MM-DD") : ""; this.setState( { invoiceList: [ ...invoiceList.slice(0, i), { ...oldInvoice, ...data }, ...invoiceList.slice(i + 1) ] }, => { this.invoiceListReduce; } );};addInvoice = ({ ...data }) => { const { invoiceList } = this.state; const { limitNum } = this.props; if (invoiceList.length && invoiceList.length >= limitNum) { Message.error("上传失败，发票数量不可超过500张"); return; } data.invoiceUrl = data.invoiceUrl || data.dataUrl || ""; delete data.dataUrl; data.invoiceDate = data.invoiceDate ? moment(data.invoiceDate).format("YYYY-MM-DD") : ""; this.setState( { invoiceList: [...invoiceList, { ...data, id: getIncrementCid }] }, => { const { invoiceList: list } = this.state; if (list.length) { this.verifyInvoiceList(list); } this.invoiceListReduce; } );};

封装细节

看到下面这段代码，大概能够想象 newValidate 出现的原因，为了文章阅读体验， 删除部分代码

这个验证函数，严重违反了单一职责，首先包含了多种校验逻辑，还承担了 submit 数据预处理、submit、error处理；不仅如此，还和视图层耦合，包括了回到顶部、定位到错误位置、错误DOM样式调整的逻辑。

当然了，看到newValidate代码行数，也没有好到哪去。

此处200多行代码就成了这个工程的毒瘤。

validate = => { const { form, totalDraftAmount, output, outputBasename } = this.props; const { validateFields } = form; const { financeChannel, orderNo: oldOrderNo, checked } = this.state; const ocrDomList = document.querySelectorAll('.ocrform'); const checkboxDomList = document.querySelectorAll('.ant-checkbox'); // 每次 validate 前先去除上次打的标, Object.values(ocrDomList)为了兼容 ie Object.values(ocrDomList).forEach(item => { item.classList.remove('field', 'error'); }); validateFields(async (err, data) => { if (err) { Message.warn('请核对填写的内容'); if (output) { // 回到顶部 scrollToTop; return; } // 定位到第一个错误 setTimeout(this.goToError(document.querySelector('.field.error'))); return; } // 验证发票 const { contractAmt, draftInfos = {}, invoiceInfos } = data; /* 此处省略20行 */ if (totalDraftAmount > contractAmt) { /* 此处省略7行 */ } // 访问子组件中的勾选状态 如没勾选，则校验不通过 const { checkedA, checkedB } = this.myRef.current.state; // 只要有一个没勾选就进来 if (!checked || !checkedA || !checkedB) { // 如果是 确认背书未勾选则 提示相应文案 Message.warn('请阅读并同意相关服务协议'); if (output) { // 回到顶部 scrollToTop; return; } // 先打标记，再定位错误 checkboxDomList[0].classList.add('error'); // 定位到第一个错误 setTimeout( this.goToError(document.querySelector('.ant-checkbox.error')) ); return; } let selectedDraftInfo = []; /* 此处省略 14 行 selectedDraftInfo 数据组装*/ const sendData = { ...data, draftInfos: selectedDraftInfo }; const { couponReleaseNo } = getLocationParams(location) || {}; if (couponReleaseNo) sendData.couponReleaseNo = couponReleaseNo; if (oldOrderNo) sendData.orgOrderNo = oldOrderNo; // 用户提交时选择为无重复背书, 删除已上传重复背书文件 const { billReusedStatus } = this.endorseBillRef.current.state; if (billReusedStatus === billReusedStatusEnum.noReuse) { delete sendData.repeatedEndorseFileUrl; } try { /* 此处省略 19 行 发起接口调用， 成功 + 失败逻辑处理*/ } catch (error) { this.setState({ loading: false }); let errMessage; // 通过 error.message 字符串中 拿到错误模块对应的 invoiceNo errMessage = error.message.split('[')[1]; if (!errMessage) return; errMessage = errMessage.split(']')[0]; // 通过报错信息定位到错误模块索引 const errorInvoiceIndex = invoiceInfos.findIndex( item => item.invoiceNo === errMessage ); // 添加标红样式 ocrDomList[errorInvoiceIndex].classList.add('field', 'error'); if (output) { // 回到顶部 scrollToTop; return; } // 定位到发票错误位置 setTimeout(this.goToError(ocrDomList[errorInvoiceIndex])); } });};

认知复杂度与圈复杂度

整体来说正相关， 也有例外：

function getWords(number) { // +1 switch (number) { case 1: // +1 return "one"; case 2: // +1 return "a couple"; case 3: // +1 return "a few"; default: return "lots"; }} // 圈复杂度：4function getWords(number) { switch (number) { // +1 case 1: return "one"; case 2: return "a couple"; case 3: return "a few"; default: return "lots"; }} // 认知复杂度：1function sumOfPrimes(max) { // +1 let total = 0; for (let i = 1; i <= max; i++) { // +1 for (let j = 2; j < i; j++) { // +1 if (i % j === 0) { // +1 continue; } } total += i; } return total;} // 圈复杂度：4function sumOfPrimes(max) { let total = 0; for (let i = 1; i <= max; i++) { // +1 for (let j = 2; j < i; j++) { // +2 if (i % j === 0) { // +3 continue; // +1 } } total += i; } return total;} // 认知复杂度：7

复杂度评判标准

需要添加“黑客代码（hack）”来保证功能的正常运行。

总是有其他开发者询问代码的某部分是如何工作的。

总是有其他开发者因为误用了你的代码而导致出现bug。

即使是有经验的开发者也无法立即读懂某行代码。

你害怕修改这一部分代码。

管理层认真考虑雇用一个以上的开发人员来处理一个类或文件。

很难搞清楚应该如何增加新功能。

如何在这部分代码中实现某些东西常常会引起开发者之间的争论。

人们常常对这部分代码做完全没有必要的修改，这通常在代码评审时，或者在变更被合并进入主干分支后才被发现。

--- 《编程原则》

规范性

这部分内容比较多，更多内容见 Code Review 手册

import 排序的例子

可以看到第一段代码，没有规律，阅读成本高，第1行, 第5行出现了重复引用。

reviewer建议：

使用工具进行格式化，提高可读性

https://github.com/lydell/eslint-plugin-simple-import-sort

https://github.com/import-js/eslint-plugin-import/

import { ref } from 'vue'import Taro from '@tarojs/taro'import gwApi from '@/api/index-gateway-js'import api from '@/api/index-js'import { onMounted, reactive, watch } from 'vue'import InputRight from '../components/InputRight.vue'import { isweapp, getParams } from '@/utils'import { FACTORY_ADDRESS_ORIGIN } from '@/utils/const.js'import { sgmReportCustom } from '@/utils/log'import { genAddressStr } from '@/utils/address'import Taro from '@tarojs/taro'import { onMounted, reactive, ref, watch } from 'vue'import api from '@/api/index.js' import gwApi from '@/api/index-gateway-js' import { getParams, isWeapp } from '@/utils' import { genAddressStr } from '@/utils/address' import { FACTORY_ADDRESS_ORIGIN } from '@/utils/const.js' import { sgmReportCustom } from '@/utils/log'import InputRight from '…./components/InputRight.vue'

命名（世上问题千千万，问题命名占一半）

不用宽泛的模块或文件名

没有拼写错误，单复数也应该正确

符合规范：

文件名kebab-case

类名PascalCase

文件作用域内 常量、变量、函数 camelCase

private 是否采用下划线，应保持一致

bad case:

// 无意义命名let array = [1, 2, 3, 4, 5]let temp = falseimport Part1 from './Part1';import Part2 from './Part2';import Part3 from './Part3';import Part4 from './Part4';// magic numberlet point = CGPoint(x: 123, у: 456)// 硬编码reportEvent("ImageClickEventId")

其他

连等

// 连等elm.onload = elm.onreadystatechange = resolveFn

一段重试逻辑

虽然 if 嵌套不多， 但是让人心智负担很重， 无法快速看出 count 值是多少会 false， 代码写的像面试题

if ((data && data.eid) || count++ > 20) { if (!data.eid) { webLog.custom({ type: 1, code: 'getEidInfo-empty', msg: data, }) } clearInterval(timer) resolve({ data })}

reviewer 建议：

使用卫语句提前剔除负向逻辑后， 虽然代码更长， 但是更好理解了。

if (!data.eid && count <= 20) { count++ return}if (!data.eid) { webLog.custom({ type: 1, code: 'getEidInfo-empty', msg: data, })}clearInterval(timer)resolve({ data })

CR落地-常见挑战

Code Review时看不出问题

参考解法：

组织集体审查讨论，提升大家审查能力，在代码质量上达成共识

代码审查方式对比

担心冲突、害怕出错

比如 A 看出了不少问题，但是发现代码作者非常不耐烦，导致 A 不敢把看到的所有问题都提出来。

参考解法：

冲突发生

解决冲突

Leader协助沟通及仲裁

协商达成共识

寻求第三人评估

组内讨论

?置若罔闻

?放任自由

预防冲突

沟通技巧

尽量疑向、不要太肯定

如果采用......是否会更合适？

?这里应该......

是否考虑过......这样的方案？

?......方案肯定更好

这个地方似乎会影响滚动性能？

?这样写肯定会影响滚动性能

发现问题，尽量提供建议

......这样会更简洁

?你这代码复杂度太高了

根据......项目规范，这里应该这样...

?你这代码不符合项目规范

特别注意

不要吝啬称赞

没必要力求完美

没有时间 CR

浇花很有意义, 但是先把火灭了

首先区分紧急与真正紧急：

CR 时间不够

工作量评估要包含 CR 时间

推荐预留 20% 的 CR 时间

权衡：

关注设计大于具体实现；

保证不出线上问题为底线；

管理好交付里程碑

越大的里程碑越容易产生大型CL，会拖慢CR速度

建议数据：400行/小时（样式、dom行可适当剔除）

建议里程碑交付周期：1周，最长2周

真正紧急情况

同步CR

写完代码当面或电话同步Review

并行CR

结对编程

紧急情况后门

google 的做法：自己是Owner，写“To be reviewed”可绕过审查

历史包袱过重

通解： 卡住增量，治理存量

CR的目的是让每一次合并都在改善代码仓库的水平

其他-提升团队工程素养

? 设计模式: 掌握24种设计模式

? 设计原则: 掌握SOLID原则（单一职责、开闭原则、里氏替换、接口隔离、依赖反转）

? 方法学: 了解Devops，极限编程，Scrum，精益，看板，瀑布，结构化分析，结构化设计

? 实践: 实践测试驱动开发，面向对象设计，结构化编程，函数式编程，持续集成，结对编程

书籍推荐

《编程原则( understanding software)》

《重构：改善既有代码的设计》

《编写可读代码的艺术》

《代码大全》

《敏捷软件开发》

《架构整洁之道》

《代码整洁之道》

相关资源

Code Review 速查手册