名词解释
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 不是当务之急，没有眼前收益 -
危害被低估，忽视“复利”的威力（非线性）
意义
CR原则
-
只要CL可以提高整体代码健康状态，就应该倾向于批准合入，即使CL并不完美 -
基于技术事实和数据的沟通 -
基于技术事实和数据否决个人偏好和意见 -
软件设计问题不能简单归结为个人偏好 -
解决冲突：不要因为无法达成一致而卡壳 -
善用工具 -
基于Lint、公司代码规范等工具 -
大模型辅助
发起CR
发起前的准备
-
推荐自己做一个 checklist -
把自己当作 Reviewer 来对自己的代码进行 CR -
预估代码可能出问题的地方 -
进行充分自测，保证代码可运行 -
不要指望别人帮你找出问题
利用自动化工具进行前置检查
-
单元测试检查 -
新增单元测试检查 -
方法行数过多 -
圈复杂度过高 -
代码规范检查 -
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
Commit 描述
Bad Case：
“修复错误”是不充分的 CL 描述。什么 bug？你做了什么来修复它？
-
逻辑修复 -
添加补丁 -
增加XX规则 -
删除XX逻辑
◆ 摘要：【xx模块】新增xx功能
◆ 背景：新功能需求，要求xxx， 详见[卡片链接]
◆ 说明：由于xx，新增xx处理模块…
-
摘要：删除 RPC 服务器消息自由列表的大小限制 -
说明：像 FizzBuzz 这样的服务器有非常大的消息，可以从重用中受益。使自由列表更大，并添加一个 goroutine 随着时间的推移慢慢释放自由列表条目，以便空闲服务器最终释放所有自由列表条目。
心态
-
一次 CR 其实是开启的一次“对话” -
应该期待评论和反馈，并及时进行回复 -
做好心理准备自己的代码可能会有缺陷 -
CR 的目的之一就是发现问题， 所以不要有抵触
CR内容
代码是写给人看的, 不是写给机器看的,只是顺便计算机可以执行而已。------《计算机程序的构造和解释》
应该被 CR 的内容：
CR流程顺序
https://google.github.io/eng-practices/review/reviewer/navigate
京东实际代码片段评审讲解
设计
应该有合理的职责划分，合理的封装
componentDidMount() {this.fetchUserInfo();this.fetchCommonInfo();this.fetchBankDesc();}
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()}
问题2，af 命名过于随意
问题3，‘300000’ 魔法字符串
问题4，选择使用 af 或 zpm 这两个URL的逻辑建议封装，避免多次调用 isSaveUrl
优秀代码设计的特质 CLEAN
• 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"><FormItemlabel="基础核验">{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"><FormItemlabel="剩余额度">{formatAmount(availableLimit)}</FormItem></div></div>) :<div className="m-b-4 row"><div className="col-11"><FormItemlabel="基础核验">{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"><FormItemlabel="基础核验">{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;
-
每个 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,}
没事别写注释
为什么接下来的代码要这么做
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() + 1let startTime = nulllet endTime = nullif (month <= 3) {startTime = year + split + '01' + split + '01' + startDateTimeendTime = year + split + '03' + split + '31' + endDateTime} else if (month > 3 && month <= 6) {startTime = year + split + '04' + split + '01' + startDateTimeendTime = year + split + '06' + split + '30' + endDateTime} else if (month > 6 && month <= 9) {startTime = year + split + '07' + split + '01' + startDateTimeendTime = year + split + '09' + split + '30' + endDateTime} else {startTime = year + split + '10' + split + '01' + startDateTimeendTime = year + split + '12' + split + '31' + endDateTime}// 本季度开始时间startTime = isTimestamp ? getTimestamp(startTime) : startTime// 本季度结束时间endTime = isTimestamp ? getTimestamp(endTime) : endTimereturn {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();});};
封装细节
这个验证函数，严重违反了单一职责，首先包含了多种校验逻辑，还承担了 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)为了兼容 ieObject.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 字符串中 拿到错误模块对应的 invoiceNoerrMessage = 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) { // +1switch (number) {case 1: // +1return "one";case 2: // +1return "a couple";case 3: // +1return "a few";default:return "lots";}} // 圈复杂度：4function getWords(number) {switch (number) { // +1case 1:return "one";case 2:return "a couple";case 3:return "a few";default:return "lots";}} // 认知复杂度：1
function sumOfPrimes(max) { // +1let total = 0;for (let i = 1; i <= max; i++) { // +1for (let j = 2; j < i; j++) { // +1if (i % j === 0) { // +1continue;}}total += i;}return total;} // 圈复杂度：4function sumOfPrimes(max) {let total = 0;for (let i = 1; i <= max; i++) { // +1for (let j = 2; j < i; j++) { // +2if (i % j === 0) { // +3continue; // +1}}total += i;}return total;} // 认知复杂度：7
复杂度评判标准
-
需要添加“黑客代码（hack）”来保证功能的正常运行。 -
总是有其他开发者询问代码的某部分是如何工作的。 -
总是有其他开发者因为误用了你的代码而导致出现bug。 -
即使是有经验的开发者也无法立即读懂某行代码。 -
你害怕修改这一部分代码。 -
管理层认真考虑雇用一个以上的开发人员来处理一个类或文件。 -
很难搞清楚应该如何增加新功能。 -
如何在这部分代码中实现某些东西常常会引起开发者之间的争论。 -
人们常常对这部分代码做完全没有必要的修改，这通常在代码评审时，或者在变更被合并进入主干分支后才被发现。
--- 《编程原则》
规范性
import 排序的例子
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 是否采用下划线，应保持一致
// 无意义命名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 ((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 })
CR落地-常见挑战
Code Review时看不出问题
组织集体审查讨论，提升大家审查能力，在代码质量上达成共识
代码审查方式对比
担心冲突、害怕出错
参考解法：
冲突发生
-
解决冲突
✅ Leader协助沟通及仲裁
✅ 协商达成共识
✅ 寻求第三人评估
✅ 组内讨论
❌置若罔闻
❌放任自由
预防冲突
-
沟通技巧
尽量疑向、不要太肯定
✅如果采用......是否会更合适？
❌这里应该......
✅是否考虑过......这样的方案？
❌......方案肯定更好
✅这个地方似乎会影响滚动性能？
❌这样写肯定会影响滚动性能 -
发现问题，尽量提供建议
✅......这样会更简洁
❌你这代码复杂度太高了
✅根据......项目规范，这里应该这样...
❌你这代码不符合项目规范
特别注意
没必要力求完美👍👍👍
没有时间 CR
浇花很有意义, 但是先把火灭了
CR 时间不够
工作量评估要包含 CR 时间
权衡：
保证不出线上问题为底线；
管理好交付里程碑
建议数据：400行/小时（样式、dom行可适当剔除）
建议里程碑交付周期：1周，最长2周
真正紧急情况
同步CR
并行CR
紧急情况后门
历史包袱过重
CR的目的是让每一次合并都在改善代码仓库的水平
其他-提升团队工程素养
✓ 设计原则: 掌握SOLID原则（单一职责、开闭原则、里氏替换、接口隔离、依赖反转）
✓ 方法学: 了解Devops，极限编程，Scrum，精益，看板，瀑布，结构化分析，结构化设计
✓ 实践: 实践测试驱动开发，面向对象设计，结构化编程，函数式编程，持续集成，结对编程
书籍推荐
《编程原则( understanding software)》
《重构：改善既有代码的设计》
《编写可读代码的艺术》
《代码大全》
《敏捷软件开发》
《架构整洁之道》
《代码整洁之道》
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
扫一扫，加入技术交流群