代码质量与技术债系列分享之一—如何做好CodeReview
Tech
01 参考资料
在今年的敏捷团队建设中,我通过Suite执行器实现了一键自动化单元测试。Juint除了Suite执行器还有哪些执行器呢?由此我的Runner探索之旅开始了!
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/54901945302 名词解释
理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。
03 CR意义
理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。
灵魂拷问:为什么我们接手的每个代码库都如此难以维护?
疲于应付眼前
不可见,意识不到
CR 非功能性开发
CR 不是当务之急,没有眼前收益
危害被低估,忽视“复利”的威力(非线性)
现代代码评审【modern Code Review】,业内认为有效的、基于最佳实践的质量保证工作流,可通过人工审代码降低风险、增强可维护性和提升研发效率,同时可以有效提升个人和团队技术能力。更是一种对代码精益求精、追求极致的态度、是“工匠精神”的一种体现。
04 CR原则
理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。
只要CL可以提高整体代码健康状态,就应该倾向于批准合入,即使CL并不完美
基于技术事实和数据的沟通
基于技术事实和数据否决个人偏好和意见
软件设计问题不能简单归结为个人偏好
解决冲突:不要因为无法达成一致而卡壳
善用工具
基于Lint、公司代码规范等工具
大模型辅助
05 发起CR
理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。
推荐自己做一个 checklist
把自己当作 Reviewer 来对自己的代码进行 CR
预估代码可能出问题的地方
进行充分自测,保证代码可运行
不要指望别人帮你找出问题
单元测试检查
新增单元测试检查
方法行数过多
圈复杂度过高
代码规范检查
lint 检查
体积监控
一次评审200LOC为佳,最多400LOC
评审量应低于500LOC/小时
一次评审不要超过60分钟
采用轻量级评审方式
全员参与代码评审
每周花费0.5~1天开展CR
严格且彻底的评审
Bad Case:
“修复错误”是不充分的 CL 描述。什么 bug?你做了什么来修复它?
其他类似的不良描述包括:
逻辑修复
添加补丁
增加XX规则
删除XX逻辑
◆ 背景:新功能需求,要求xxx, 详见[卡片链接]
◆ 说明:由于xx,新增xx处理模块…
摘要:删除 RPC 服务器消息自由列表的大小限制
说明:像 FizzBuzz 这样的服务器有非常大的消息,可以从重用中受益。使自由列表更大,并添加一个 goroutine 随着时间的推移慢慢释放自由列表条目,以便空闲服务器最终释放所有自由列表条目。
必要时,应使用 cz 等工具进行规范。
一次 CR 其实是开启的一次“对话”
应该期待评论和反馈,并及时进行回复
做好心理准备自己的代码可能会有缺陷
CR 的目的之一就是发现问题, 所以不要有抵触
06 CR内容
理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。
代码是写给人看的, 不是写给机器看的,只是顺便计算机可以执行而已。------《计算机程序的构造和解释》
自上而下,优先级从高到低:
https://google.github.io/eng-practices/review/reviewer/navigate
设计
应该有合理的职责划分,合理的封装
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’ 魔法字符串
优秀代码设计的特质 CLEAN
Cohesive:内聚的代码更容易理解和查找bug• Loosely Coupled:松耦合的代码让实体之间的副作用更少,更容易测试、复用、扩展• Encapsulated:封装良好的代码有助于管理复杂度,也更容易修改• Assertive:自主的代码其行为和其所依赖的数据放在一起,不与其它代码互相干预(Tell but not Ask)• Nonredundant:无冗余的代码意味着可以只在一个地方修复bug和进行更改
应避免过度设计
功能
逻辑正确,逻辑合理,避免晦涩难懂的逻辑
{ 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>) :
['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> :
null}
魔法字符串, 且重复出现
['11', '12'].indexOf(invoiceType) === -1
无意义的空行,严重影响代码阅读 FormItem 重复过多
const isNotOnlineInvoice = ['11', '12'].indexOf(invoiceType) === -1;
安全性
// 微信服务号 生产配置中复写
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:为什么接下来的代码要这么做
复杂度 优先使用标准库中的能力
封装细节
写的代码越简单, 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
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代码行数,也没有好到哪去。此处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";
}
} // 圈复杂度:4
function getWords(number) {
switch (number) { // +1
case 1:
return "one";
case 2:
return "a couple";
case 3:
return "a few";
default:
return "lots";
}
} // 认知复杂度:1
function 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;
} // 圈复杂度:4
function 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
复杂度评判标准
1.需要添加“黑客代码(hack)”来保证功能的正常运行。
2.总是有其他开发者询问代码的某部分是如何工作的。
3.总是有其他开发者因为误用了你的代码而导致出现bug。
4.即使是有经验的开发者也无法立即读懂某行代码。
5.你害怕修改这一部分代码。
6.管理层认真考虑雇用一个以上的开发人员来处理一个类或文件。
7.很难搞清楚应该如何增加新功能。
8.如何在这部分代码中实现某些东西常常会引起开发者之间的争论。
规范性
import 排序的例子
https://github.com/lydell/eslint-plugin-simple-import-sort
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 是否采用下划线,应保持一致
// 无意义命名
let array = [1, 2, 3, 4, 5]
let temp = false
import Part1 from './Part1';
import Part2 from './Part2';
import Part3 from './Part3';
import Part4 from './Part4';
// magic number
let point = CGPoint(x: 123, у: 456)
// 硬编码
reportEvent("ImageClickEventId")
其他
连等 // 连等
elm.onload = elm.onreadystatechange = resolveFn
一段重试逻辑
if ((data && data.eid) || count++ > 20) {
if (!data.eid) {
webLog.custom({
type: 1,
code: 'getEidInfo-empty',
msg: data,
})
}
clearInterval(timer)
resolve({ data })
}
if (!data.eid && count <= 20) {
count++
return
}
if (!data.eid) {
webLog.custom({
type: 1,
code: 'getEidInfo-empty',
msg: data,
})
}
clearInterval(timer)
resolve({ data })
07 CR落地—常见挑战
理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。
代码审查方式对比
冲突发生 解决冲突 ✅ Leader协助沟通及仲裁
✅ 协商达成共识
✅ 寻求第三人评估
✅ 组内讨论
❌置若罔闻
❌放任自由 预防冲突 沟通技巧
尽量疑向、不要太肯定
✅如果采用......是否会更合适?
❌这里应该......
✅是否考虑过......这样的方案?
❌......方案肯定更好
✅这个地方似乎会影响滚动性能?
发现问题,尽量提供建议
✅......这样会更简洁
❌你这代码复杂度太高了
✅根据......项目规范,这里应该这样...
特别注意
不要吝啬称赞👍👍👍
首先区分紧急与真正紧急:
CR 时间不够 工作量评估要包含 CR 时间
推荐预留 20% 的 CR 时间
权衡:
关注设计大于具体实现;
保证不出线上问题为底线;
管理好交付里程碑
越大的里程碑越容易产生大型CL,会拖慢CR速度
建议数据:400行/小时(样式、dom行可适当剔除)
真正紧急情况 同步CR
写完代码当面或电话同步Review
并行CR
结对编程
紧急情况后门
通解: 卡住增量,治理存量
08 其他—提升团队工程素养
理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。
✓ 设计模式: 掌握24种设计模式
✓ 设计原则: 掌握SOLID原则(单一职责、开闭原则、里氏替换、接口隔离、依赖反转)
✓ 方法学: 了解Devops,极限编程,Scrum,精益,看板,瀑布,结构化分析,结构化设计
代码手术刀—自定义你的代码重构工具记一次生成慢sql索引优化及思考京东流水线——满足你对工作流编排的一切幻想
求分享
求点赞
求在看