专栏名称: 高可用架构
高可用架构公众号。
目录
相关文章推荐
程序员小灰  ·  这款AI编程工具,将会取代Cursor! ·  昨天  
汽车最前线  ·  最低9万不到,综合续航1300km+,这些国 ... ·  昨天  
玩车教授  ·  短视频 | 如何优雅地开走一台超跑? ·  2 天前  
汽车之家  ·  新7系停在你车后,是种什么体验? ·  3 天前  
宝马客  ·  非常抢眼!国内到店实拍爪哇绿宝马M2! ·  5 天前  
51好读  ›  专栏  ›  高可用架构

代码质量与技术债系列分享之一—如何做好CodeReview

高可用架构  · 公众号  ·  · 2024-04-19 10:37

正文


01
参考资料

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


02
名词解释


理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。


03
CR意义


理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。


灵魂拷问:为什么我们接手的每个代码库都如此难以维护?

重要原因之一:Code Review 执行不彻底万能借口:太忙!
  • 疲于应付眼前

  • 不可见,意识不到

  • CR 非功能性开发

  • CR 不是当务之急,没有眼前收益

  • 危害被低估,忽视“复利”的威力(非线性)
意义

现代代码评审【modern Code Review】,业内认为有效的、基于最佳实践的质量保证工作流,可通过人工审代码 降低风险、增强可维护性和提升研发效率 ,同时可以有效 提升个人和团队技术能力 更是一种对代码精益求精、追求极致的态度、是“工匠精神”的一种体现。



04
CR原则


理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。
  • 只要CL可以提高整体代码健康状态,就应该倾向于批准合入,即使CL并不完美

  • 基于技术事实和数据的沟通

    • 基于技术事实和数据否决个人偏好和意见

    • 软件设计问题不能简单归结为个人偏好

  • 解决冲突:不要因为无法达成一致而卡壳

  • 善用工具

    • 基于Lint、公司代码规范等工具

    • 大模型辅助



05
发起CR


理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。
发起前的准备
  • 推荐自己做一个 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
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 的目的之一就是发现问题, 所以不要有抵触



06
CR内容


理解,首先 MCube 会依据模板缓存状态判断是否需要网络获取最新模板,当获取到模板后进行模板加载,加载阶段会将产物转换为视图树的结构,转换完成后将通过表达式引擎解析表达式并取得正确的值,通过事件解析引擎解析用户自定义事件并完成事件的绑定,完成解析赋值以及事件绑定后进行视图的渲染,最终将目标页面展示到屏幕。

代码是写给人看的, 不是写给机器看的,只是顺便计算机可以执行而已。------《计算机程序的构造和解释》

应该被 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 ? (    
label="基础核验" >
{basicVerifyStatus ? '已通过' :
未通过
}
label="剩余额度" > {formatAmount(availableLimit)}
) :
label="基础核验" >
{basicVerifyStatus ? '已通过' :
未通过
}
) :
['11', '12'].indexOf(invoiceType) === -1 ?
label="基础核验" >
{basicVerifyStatus ? '已通过' :
未通过
}
:
null}
关键问题:连续三元判断 + 嵌套三元判断
其他问题:
  • 魔法字符串, 且重复出现
['11', '12'].indexOf(invoiceType) === -1
  • 无意义的空行,严重影响代码阅读
  • FormItem 重复过多
Reviewer 建议:
1.对重复代码,梳理内容,进行合理命名
const isNotOnlineInvoice = ['11', '12'].indexOf(invoiceType) === -1;
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";  }}                                       // 圈复杂度: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.如何在这部分代码中实现某些东西常常会引起开发者之间的争论。

9.人们常常对这部分代码做完全没有必要的修改,这通常在代码评审时,或者在变更被合并进入主干分支后才被发现。--- 《编程原则》
  • 规范性
这部分内容比较多,更多内容见 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







请到「今天看啥」查看全文


推荐文章
程序员小灰  ·  这款AI编程工具,将会取代Cursor!
昨天
汽车之家  ·  新7系停在你车后,是种什么体验?
3 天前
新京报传媒研究  ·  我们时代的阅读渴求与知识焦虑
7 年前
CMKT咨询圈  ·  有一种失败叫瞎忙
7 年前
阿何有话说  ·  什么样的女孩最撩人?
7 年前