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

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

代码审查（Code Review）是软件开发过程中至关重要的一环，它不仅能帮助团队提升代码质量，还能促进知识共享和团队协作。本文将从Code Review的意义、原则、发起流程、具体内容和心态等方面，为你提供一份详尽的指南。

Code Review 速查手册

参考资料

• Too Busy to Improve
• WTF per Minute
• ACM Digital Library
• Google Engineering Practices
• 豆瓣图书
• 知乎专栏

名词解释

• CR: Code Review
• CL: 变更列表（Changelist），表示已提交到版本控制或正在进行代码审查的独立更改。其他组织通常将其称为“更改”、“补丁”或“拉取请求”。
• LGTM: "Looks Good to Me"，这是代码审查员在批准 CL 时所说的。

CR意义

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

重要原因之一：Code Review 执行不彻底

万能借口：太忙！

• 疲于应付眼前
• 不可见，意识不到
• CR 非功能性开发
• CR 不是当务之急，没有眼前收益
• 危害被低估，忽视“复利”的威力（非线性）

意义

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

CR原则

• 只要CL可以提高整体代码健康状态，就应该倾向于批准合入，即使CL并不完美
• 基于技术事实和数据的沟通
• 基于技术事实和数据否决个人偏好和意见
• 软件设计问题不能简单归结为个人偏好
• 解决冲突：不要因为无法达成一致而卡壳
• 善用工具
• 基于Lint、公司代码规范等工具
• 大模型辅助

发起CR

发起前的准备

• 推荐自己做一个 checklist
• 把自己当作 Reviewer 来对自己的代码进行 CR
• 预估代码可能出问题的地方
• 进行充分自测，保证代码可运行
• 不要指望别人帮你找出问题
  checklist 可参考Code Review 速查手册

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

• 单元测试检查
• 新增单元测试检查
• 方法行数过多
• 圈复杂度过高
• 代码规范检查
• lint 检查
• 体积监控
  建议平均时长不要超过10分钟， 所以 e2e，性能检查等建议不阻塞发起MR流程

合理的规模

最佳实践

• 一次评审200LOC为佳，最多400LOC
• 评审量应低于500LOC/小时
• 一次评审不要超过60分钟
• 采用轻量级评审方式
• 全员参与代码评审
• 每周花费0.5~1天开展CR
• 严格且彻底的评审

如何拆分 CL

Google Engineering Practices

Commit 描述

Bad Case：

“修复错误”是不充分的 CL 描述。什么 bug？你做了什么来修复它？
其他类似的不良描述包括：

• 逻辑修复
• 添加补丁
• 增加XX规则
• 删除XX逻辑

Good Case：

• 摘要：【xx模块】新增xx功能
• 背景： 新功能需求，要求xxx， 详见[卡片链接]
• 说明： 由于xx，新增xx处理模块…
• 摘要：删除 RPC 服务器消息自由列表的大小限制
• 说明：像 FizzBuzz 这样的服务器有非常大的消息，可以从重用中受益。使自由列表更大，并添加一个 goroutine 随着时间的推移慢慢释放自由列表条目，以便空闲服务器最终释放所有自由列表条目。
  必要时，应使用 cz 等工具进行规范。

心态

• 一次 CR 其实是开启的一次“对话”
• 应该期待评论和反馈，并及时进行回复
• 做好心理准备自己的代码可能会有缺陷
• CR 的目的之一就是发现问题， 所以不要有抵触

CR内容

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

应该被 CR 的内容：

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

• 设计：代码是否设计良好并适合您的系统？
• 功能：代码的行为是否符合作者的预期？代码的行为方式对其用户有好处？
• 安全性：代码是否安全合规？
• 复杂性：代码可以更简单吗？当其他开发人员将来遇到此代码时，他们是否能够轻松理解和使用此代码？
• 测试：代码是否具有正确且设计良好的自动化测试？
• 命名：开发人员是否为变量、类、方法等选择了明确的名称？
• 注释：注释是否清晰有用？
• 风格：代码是否遵循京东代码风格规范？
• 文档：开发人员是否更新了相关文档？

Google Engineering Practices

CR流程顺序

Google Engineering Practices

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

设计

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

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