devbox
很楠不爱3
这个屌丝很懒，什么也没留下！
关注作者
热门标签
热门文章
当前位置:   article > 正文

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

作者：很楠不爱3 | 2024-06-08 15:04:43

赞

踩

如何做code review

01 

参考资料

元测试。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/549019453

02 

  名词解释  

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

19eac3710bcd91874de9a0185cb87ba0.png

03 

  CR意义  

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

灵魂拷问：为什么我们接手的每个代码库都如此难以维护？c33ce6ed3fd55e541eee02b792851989.png

重要原因之一：Code Review 执行不彻底万能借口：太忙！

  • 疲于应付眼前

  • 不可见，意识不到

  • CR 非功能性开发

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

  • 危害被低估，忽视“复利”的威力（非线性）

意义

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

04 

  CR原则  

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

  • 只要CL可以提高整体代码健康状态，就应该倾向于批准合入，即使CL并不完美

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

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

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

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

  • 善用工具

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

    • 大模型辅助

05 

  发起CR  

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

发起前的准备

  • 推荐自己做一个 checklist

  • 把自己当作 Reviewer 来对自己的代码进行 CR

  • 预估代码可能出问题的地方

  • 进行充分自测，保证代码可运行

  • 不要指望别人帮你找出问题

利用自动化工具进行前置检查

  • 单元测试检查

  • 新增单元测试检查

  • 方法行数过多

  • 圈复杂度过高

  • 代码规范检查

  • lint 检查

  • 体积监控

建议平均时长不要超过10分钟， 所以 e2e，性能检查等建议不阻塞发起MR流程

合理的规模

b10680f1ced01116fb71aca4fa9867be.png6b251fa4527f35d8de1e0cce227672b8.png

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：

33a4284cffe13042d47973863871e0ae.png

“修复错误”是不充分的 CL 描述。什么 bug？你做了什么来修复它？

其他类似的不良描述包括：

  • 逻辑修复

  • 添加补丁

  • 增加XX规则

  • 删除XX逻辑

Good Case：

◆ 摘要：【xx模块】新增xx功能
◆ 背景：新功能需求，要求xxx， 详见[卡片链接]
◆ 说明：由于xx，新增xx处理模块…

    • 摘要：删除 RPC 服务器消息自由列表的大小限制

    • 说明：像 FizzBuzz 这样的服务器有非常大的消息，可以从重用中受益。使自由列表更大，并添加一个 goroutine 随着时间的推移慢慢释放自由列表条目，以便空闲服务器最终释放所有自由列表条目。

必要时，应使用 cz 等工具进行规范。

心态

  • 一次 CR 其实是开启的一次“对话”

  • 应该期待评论和反馈，并及时进行回复

  • 做好心理准备自己的代码可能会有缺陷

  • CR 的目的之一就是发现问题， 所以不要有抵触

06 

  CR内容  

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

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

应该被 CR 的内容：

自上而下，优先级从高到低：

86f8fc895b722f2beb6efc15cb9222d8.png

65364d89202fa90fd35a80878ce6f659.png

https://google.github.io/eng-practices/review/reviewer/looking-for.html

CR流程顺序

5ffc72ca0801a9750c71b4a1659dfc47.png

https://google.github.io/eng-practices/review/reviewer/navigate

京东实际代码片段评审讲解

  • 设计

    • 应该有合理的职责划分，合理的封装

good case

  1. componentDidMount() {
  2.   this.fetchUserInfo();
  3.   this.fetchCommonInfo();
  4.   this.fetchBankDesc();
  5. }

bad case

  1. componentDidMount() {
  2.   const { location, dispatch, accountInfoList } = this.props;
  3.   const { token, af } = getLocationParams(location) || {};
  4.   dispatch({
  5.     type: 'zpmUserCenter/fetchUserInfo',
  6.     payload: {
  7.       token,
  8.     },
  9.   }).catch(e => {
  10.     const zpmOpenAuthUrlLogin = decodeURIComponent(getCookie('zpmOpenAuthUrlLogin'));
  11.     // 如果token过期则跳转回第三方平台
  12.     if ([TOKEN_EXPIRED_CODE, '300000'].includes(e.errorCode) && (isSaveUrl(af) || isSaveUrl(zpmOpenAuthUrlLogin))) {
  13.       setTimeout(() => {
  14.         window.location.href = isSaveUrl(af) ? af : zpmOpenAuthUrlLogin;
  15.       }, 2000);
  16.     }
  17.   });
  18.  
  19.  
  20.   if (!this.showWhichHeader() && !this.showGatewayHeader()) {
  21.     dispatch({
  22.       type: 'zpmUserCenter/fetchAccountInfo',
  23.       payload: {
  24.         token,
  25.       },
  26.     });
  27.   }
  28.   this.getBlackList()
  29. }

问题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：一段表单代码（原代码过长，仅贴出其中典型的一段）

  1. { hasQuota ? (
  2.   ['11', '12'].indexOf(invoiceType) === -1 ? (
  3.     <div className="m-b-4 row">
  4.       <div className="col-11">
  5.         <FormItem
  6.           label="基础核验"
  7.         >
  8.  
  9.  
  10.           {basicVerifyStatus ? '已通过' : <div>
  11.             未通过
  12.             <Tooltip title={basicVerifyMsg} placement="bottom">
  13.               <span className='iconfont icon-tishi theme-primary m-l-1 font-14' />
  14.             </Tooltip>
  15.           </div>}
  16.         </FormItem>
  17.       </div>
  18.       <div className="col-11">
  19.         <FormItem
  20.           label="剩余额度"
  21.         >
  22.           {formatAmount(availableLimit)}
  23.         </FormItem>
  24.       </div>
  25.     </div>) :
  26.     <div className="m-b-4 row">
  27.       <div className="col-11">
  28.         <FormItem
  29.           label="基础核验"
  30.         >
  31.  
  32.  
  33.           {basicVerifyStatus ? '已通过' : <div>
  34.             未通过
  35.             <Tooltip title={basicVerifyMsg} placement="bottom">
  36.               <span className='iconfont icon-tishi theme-primary m-l-1 font-14' />
  37.             </Tooltip>
  38.           </div>}
  39.         </FormItem>
  40.       </div>
  41.     </div>) :  
  42.   ['11', '12'].indexOf(invoiceType) === -1 ? <div className="m-b-4 row">
  43.     <div className="col-11">
  44.       <FormItem
  45.         label="基础核验"
  46.       >
  47.  
  48.  
  49.         {basicVerifyStatus ? '已通过' : <div>
  50.           未通过
  51.           <Tooltip title={basicVerifyMsg} placement="bottom">
  52.             <span className='iconfont icon-tishi theme-primary m-l-1 font-14' />
  53.           </Tooltip>
  54.         </div>}
  55.       </FormItem>
  56.     </div>
  57.   </div> :
  58.     null}

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

其他问题：

  • 魔法字符串， 且重复出现

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

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

  • FormItem 重复过多

Reviewer 建议：

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

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

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

c946cbfacc4e647adfd92939fcaad86a.png

  • 安全性

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

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

  • 一致性

    • 代码一致性：

      • 函数名和实现一致

      • 注释和代码一致

    • 命名方式一致

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

    • 抽象层级一致

    • 不建议混用 import 和 require

    • 注释与代码不一致

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

    • 命名不一致

      1. this.state = {
      2.     isOffline: false, 
      3.     shouldShowFollowLink: false, 
      4.     shouldShowToast: false, 
      5.     ifReceiveNotify: false, 
      6.     bShowAllDocsRedPoint: false,
      7.     isNewPushNotify: false,
      8. }

    • 没事别写注释

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

bad case：接下来的代码做了什么

  • 复杂度

    • 优先使用标准库中的能力

    • 封装细节

    • 写的代码越简单, bug越少

    • 尽量遵守单一职责原则

    • DRY——Don’t Repeat Yourself

    • 无意义的函数封装

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

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

  1. // 本季度开始时间、结束时间，返回毫秒值
  2. export const getQuarterStartAndEndTime = ({
  3.   time = null,
  4.   isTimestamp = true,
  5.   split = '/',
  6.   startDateTime = ' 00:00:00',
  7.   endDateTime = ' 23:59:59',
  8. } = {}) => {
  9.   let date = checkDate(time) ? new Date(time) : new Date()
  10.   let year = date.getFullYear()
  11.   let month = date.getMonth() + 1
  12.   let startTime = null
  13.   let endTime = null
  14.   if (month <= 3) {
  15.     startTime = year + split + '01' + split + '01' + startDateTime
  16.     endTime = year + split + '03' + split + '31' + endDateTime
  17.   } else if (month > 3 && month <= 6) {
  18.     startTime = year + split + '04' + split + '01' + startDateTime
  19.     endTime = year + split + '06' + split + '30' + endDateTime
  20.   } else if (month > 6 && month <= 9) {
  21.     startTime = year + split + '07' + split + '01' + startDateTime
  22.     endTime = year + split + '09' + split + '30' + endDateTime
  23.   } else {
  24.     startTime = year + split + '10' + split + '01' + startDateTime
  25.     endTime = year + split + '12' + split + '31' + endDateTime
  26.   }
  27.  
  28.  
  29.   // 本季度开始时间
  30.   startTime = isTimestamp ? getTimestamp(startTime) : startTime
  31.  
  32.  
  33.   // 本季度结束时间
  34.   endTime = isTimestamp ? getTimestamp(endTime) : endTime
  35.  
  36.  
  37.   return {
  38.     startTime,
  39.     endTime,
  40.   }
  41. }

    • DRY——Don’t Repeat Yourself

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

  1. handleMergedInvoice = selectedRows => {
  2.   const { invoiceList } = this.state;
  3.   const { limitNum } = this.props;
  4.  
  5.  
  6.   const newInvoiceList = [];
  7.   for (const item of selectedRows) {
  8.     if (newInvoiceList.length && newInvoiceList.length >= limitNum) {
  9.       Message.error(`上传失败，发票数量不可超过${limitNum}张`);
  10.       return;
  11.     }
  12.     item.invoiceUrl = item.invoiceUrl || item.dataUrl || "";
  13.     delete item.dataUrl;
  14.     item.invoiceDate = item.invoiceDate
  15.       ? moment(item.invoiceDate).format("YYYY-MM-DD")
  16.       : "";
  17.     newInvoiceList.push({ ...item, id: getIncrementCid() });
  18.     this.setState({ invoiceList: newInvoiceList }, () => {
  19.       const { invoiceList: list, errIndexList } = this.state;
  20.       if (list.length) {
  21.         this.verifyInvoiceList(list);
  22.       }
  23.       this.invoiceListReduce();
  24.     });
  25.   }
  26. };
  1. updateInvoice = ({ ...data }, i) => {
  2.   const { invoiceList } = this.state;
  3.   const oldInvoice = invoiceList[i];
  4.   data.invoiceUrl = data.invoiceUrl || data.dataUrl || "";
  5.   delete data.dataUrl;
  6.   data.invoiceDate = data.invoiceDate
  7.     ? moment(data.invoiceDate).format("YYYY-MM-DD")
  8.     : "";
  9.   this.setState(
  10.     {
  11.       invoiceList: [
  12.         ...invoiceList.slice(0, i),
  13.         { ...oldInvoice, ...data },
  14.         ...invoiceList.slice(i + 1)
  15.       ]
  16.     },
  17.     () => {
  18.       this.invoiceListReduce();
  19.     }
  20.   );
  21. };
  1. addInvoice = ({ ...data }) => {
  2.   const { invoiceList } = this.state;
  3.   const { limitNum } = this.props;
  4.   if (invoiceList.length && invoiceList.length >= limitNum) {
  5.     Message.error("上传失败，发票数量不可超过500张");
  6.     return;
  7.   }
  8.   data.invoiceUrl = data.invoiceUrl || data.dataUrl || "";
  9.   delete data.dataUrl;
  10.   data.invoiceDate = data.invoiceDate
  11.     ? moment(data.invoiceDate).format("YYYY-MM-DD")
  12.     : "";
  13.  
  14.  
  15.   this.setState(
  16.     {
  17.       invoiceList: [...invoiceList, { ...data, id: getIncrementCid() }]
  18.     },
  19.     () => {
  20.       const { invoiceList: list } = this.state;
  21.       if (list.length) {
  22.         this.verifyInvoiceList(list);
  23.       }
  24.       this.invoiceListReduce();
  25.     }
  26.   );
  27. };

    • 封装细节

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

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

当然了，看到newValidate代码行数，也没有好到哪去。此处200多行代码就成了这个工程的毒瘤。

67a274c268897d9820f55cc4a5352bf2.png

  1. validate = () => {
  2.   const { form, totalDraftAmount, output, outputBasename } = this.props;
  3.   const { validateFields } = form;
  4.   const { financeChannel, orderNo: oldOrderNo, checked } = this.state;
  5.   const ocrDomList = document.querySelectorAll('.ocrform');
  6.   const checkboxDomList = document.querySelectorAll('.ant-checkbox');
  7.   // 每次 validate 前先去除上次打的标, Object.values(ocrDomList)为了兼容 ie
  8.   Object.values(ocrDomList).forEach(item => {
  9.     item.classList.remove('field', 'error');
  10.   });
  11.   validateFields(async (err, data) => {
  12.     if (err) {
  13.       Message.warn('请核对填写的内容');
  14.       if (output) {
  15.         // 回到顶部
  16.         scrollToTop();
  17.         return;
  18.       }
  19.       // 定位到第一个错误
  20.       setTimeout(this.goToError(document.querySelector('.field.error')));
  21.       return;
  22.     }
  23.     // 验证发票
  24.     const { contractAmt, draftInfos = {}, invoiceInfos } = data;
  25.     /* 此处省略20行 */
  26.     if (totalDraftAmount > contractAmt) {
  27.       /* 此处省略7行 */
  28.     }
  29.     // 访问子组件中的勾选状态 如没勾选，则校验不通过
  30.     const { checkedA, checkedB } = this.myRef.current.state;
  31.     // 只要有一个没勾选就进来
  32.     if (!checked || !checkedA || !checkedB) {
  33.       // 如果是 确认背书未勾选则 提示相应文案
  34.       Message.warn('请阅读并同意相关服务协议');
  35.       if (output) {
  36.         // 回到顶部
  37.         scrollToTop();
  38.         return;
  39.       }
  40.       // 先打标记，再定位错误
  41.       checkboxDomList[0].classList.add('error');
  42.       // 定位到第一个错误
  43.       setTimeout(
  44.         this.goToError(document.querySelector('.ant-checkbox.error'))
  45.       );
  46.       return;
  47.     }
  48.     let selectedDraftInfo = [];
  49.     /* 此处省略 14 行 selectedDraftInfo 数据组装*/
  50.     const sendData = {
  51.       ...data,
  52.       draftInfos: selectedDraftInfo
  53.     };
  54.     const { couponReleaseNo } = getLocationParams(location) || {};
  55.     if (couponReleaseNo) sendData.couponReleaseNo = couponReleaseNo;
  56.     if (oldOrderNo) sendData.orgOrderNo = oldOrderNo;
  57.     // 用户提交时选择为无重复背书, 删除已上传重复背书文件
  58.     const { billReusedStatus } = this.endorseBillRef.current.state;
  59.     if (billReusedStatus === billReusedStatusEnum.noReuse) {
  60.       delete sendData.repeatedEndorseFileUrl;
  61.     }
  62.     try {
  63.       /* 此处省略 19 行 发起接口调用， 成功 + 失败逻辑处理*/
  64.     } catch (error) {
  65.       this.setState({
  66.         loading: false
  67.       });
  68.       let errMessage;
  69.       // 通过 error.message 字符串中 拿到错误模块对应的 invoiceNo
  70.       errMessage = error.message.split('[')[1];
  71.       if (!errMessage) return;
  72.       errMessage = errMessage.split(']')[0];
  73.       // 通过报错信息定位到错误模块索引
  74.       const errorInvoiceIndex = invoiceInfos.findIndex(
  75.         item => item.invoiceNo === errMessage
  76.       );
  77.       // 添加标红样式
  78.       ocrDomList[errorInvoiceIndex].classList.add('field', 'error');
  79.       if (output) {
  80.         // 回到顶部
  81.         scrollToTop();
  82.         return;
  83.       }
  84.       // 定位到发票错误位置
  85.       setTimeout(this.goToError(ocrDomList[errorInvoiceIndex]));
  86.     }
  87.   });
  88. };

    • 认知复杂度与圈复杂度

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

  1. function getWords(number) {             // +1
  2.   switch (number) {
  3.     case 1:                             // +1
  4.       return "one";
  5.     case 2:                             // +1
  6.       return "a couple";
  7.     case 3:                             // +1
  8.       return "a few";
  9.     default:
  10.       return "lots";
  11.   }
  12. }                                       // 圈复杂度：4
  13.  
  14.  
  15. function getWords(number) {             
  16.   switch (number) {                     // +1
  17.     case 1:                   
  18.       return "one";
  19.     case 2:                 
  20.       return "a couple";
  21.     case 3:                     
  22.       return "a few";
  23.     default:
  24.       return "lots";
  25.   }
  26. }                                       // 认知复杂度：1
  1. function sumOfPrimes(max) {             // +1
  2.   let total = 0;
  3.   for (let i = 1; i <= max; i++) {      // +1
  4.     for (let j = 2; j < i; j++) {       // +1
  5.       if (i % j === 0) {                // +1
  6.         continue;
  7.       }
  8.     }
  9.     total += i;
  10.   }
  11.   return total;
  12. }                                       // 圈复杂度：4
  13.  
  14.  
  15. function sumOfPrimes(max) {             
  16.   let total = 0;
  17.   for (let i = 1; i <= max; i++) {      // +1
  18.     for (let j = 2; j < i; j++) {       // +2
  19.       if (i % j === 0) {                // +3
  20.         continue;                       // +1
  21.       }
  22.     }
  23.     total += i;
  24.   }
  25.   return total;
  26. }                                       // 认知复杂度：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/

  1. import { ref } from 'vue'
  2. import Taro from '@tarojs/taro'
  3. import gwApi from '@/api/index-gateway-js'
  4. import api from '@/api/index-js'
  5. import { onMounted, reactive, watch } from 'vue'
  6. import InputRight from '../components/InputRight.vue'
  7. import { isweapp, getParams } from '@/utils'
  8. import { FACTORY_ADDRESS_ORIGIN } from '@/utils/const.js'
  9. import { sgmReportCustom } from '@/utils/log'
  10. import { genAddressStr } from '@/utils/address'
  1. import Taro from '@tarojs/taro'
  2. import { onMounted, reactive, ref, watch } from 'vue'
  3.  
  4.  
  5. import api from '@/api/index.js' 
  6. import gwApi from '@/api/index-gateway-js' 
  7. import { getParams, isWeapp } from '@/utils' 
  8. import { genAddressStr } from '@/utils/address' 
  9. import { FACTORY_ADDRESS_ORIGIN } from '@/utils/const.js' 
  10. import { sgmReportCustom } from '@/utils/log'
  11.  
  12.  
  13. import InputRight from '…./components/InputRight.vue'

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

      • 不用宽泛的模块或文件名

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

      • 符合规范：

        • 文件名kebab-case

        • 类名PascalCase

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

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

bad case:

  1. // 无意义命名
  2. let array = [1, 2, 3, 4, 5]
  3. let temp = false
  4. import Part1 from './Part1';
  5. import Part2 from './Part2';
  6. import Part3 from './Part3';
  7. import Part4 from './Part4';
  8.  
  9.  
  10. // magic number
  11. let point = CGPoint(x: 123, у: 456)
  12.  
  13.  
  14. // 硬编码
  15. reportEvent("ImageClickEventId")

  • 其他

    • 连等

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

    • 一段重试逻辑

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

  1. if ((data && data.eid) || count++ > 20) {
  2.   if (!data.eid) {
  3.     webLog.custom({
  4.       type: 1,
  5.       code: 'getEidInfo-empty',
  6.       msg: data,
  7.     })
  8.   }
  9.   clearInterval(timer)
  10.   resolve({ data })
  11. }

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

  1. if (!data.eid && count <= 20) {
  2.   count++
  3.   return
  4. }
  5. if (!data.eid) {
  6.   webLog.custom({
  7.     type: 1,
  8.     code: 'getEidInfo-empty',
  9.     msg: data,
  10.   })
  11. }
  12.  
  13.  
  14. clearInterval(timer)
  15. resolve({ data })

07 

  CR落地—常见挑战  

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

Code Review时看不出问题

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

代码审查方式对比

543116c4f507140c93a1badc93b92e6d.png

担心冲突、害怕出错

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

参考解法：

  • 冲突发生

    • 解决冲突

      ✅ Leader协助沟通及仲裁

      ✅ 协商达成共识

      ✅ 寻求第三人评估

      ✅ 组内讨论

      ❌置若罔闻

      ❌放任自由

  • 预防冲突

    • 沟通技巧

        尽量疑向、不要太肯定

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

        ❌这里应该......

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

        ❌......方案肯定更好

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

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

    • 发现问题，尽量提供建议

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

        ❌你这代码复杂度太高了

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

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

  • 特别注意

不要吝啬称赞

声明：本文内容由网友自发贡献，不代表【wpsshop博客】立场，版权归原作者所有，本站不承担相应法律责任。如您发现有侵权的内容，请联系我们。转载请注明出处：https://www.wpsshop.cn/w/很楠不爱3/article/detail/690166
推荐阅读
相关标签

Copyright © 2003-2013 www.wpsshop.cn 版权所有，并保留所有权利。