百度360必应搜狗淘宝本站头条
当前位置:网站首页 > 文章教程 > 正文

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

xsobi 2025-01-01 23:16 31 浏览

TL;DR

Code Review 速查手册

参考资料

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

名词解释

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 不是当务之急,没有眼前收益
  • 危害被低估,忽视“复利”的威力(非线性)

意义

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

CR原则

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

发起CR

发起前的准备

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

checklist 可参考Code Review 速查手册

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

  • 单元测试检查
  • 新增单元测试检查
  • 方法行数过多
  • 圈复杂度过高
  • 代码规范检查
  • 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 的目的之一就是发现问题, 所以不要有抵触

CR内容

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

应该被 CR 的内容:

自上而下,优先级从高到低:

模块

简介

设计

代码是否设计良好并适合您的系统?

功能

代码的行为是否符合作者的预期?代码的行为方式对其用户有好处?

安全性

代码是否安全合规?

复杂性

代码可以更简单吗?当其他开发人员将来遇到此代码时,他们是否能够轻松理解和使用此代码?

测试

代码是否具有正确且设计良好的自动化测试?

命名

开发人员是否为变量、类、方法等选择了明确的名称?

注释

注释是否清晰有用?

风格

代码是否遵循京东代码风格规范?

文档

开发人员是否更新了相关文档?

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

CR流程顺序


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

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

设计

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

good case

Bash
componentDidMount() {
  this.fetchUserInfo();
  this.fetchCommonInfo();
  this.fetchBankDesc();
}

bad case

Bash
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>) :  
  ['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 重复过多

Reviewer 建议:

  1. 对重复代码,梳理内容,进行合理命名
const isNotOnlineInvoice = ['11', '12'].indexOf(invoiceType) === -1;
  1. 每个 FormItem 也进行命名,三元逻辑梳理,重构


isNotOnlineInvoice true

isNotOnlineInvoice false

hasQuota true

verifyCodeFormItem
invoiceCodeFormItem
goodNameFormItem
modifiedDateFormItem
basicVerifyFormItem
availableLimitFormItem

availableLimitFormItem
modifiedDateFormItem
basicVerifyFormItem

hasQuota false

verifyCodeFormItem
invoiceCodeFormItem
goodNameFormItem
modifiedDateFormItem
basicVerifyFormItem

basicVerifyFormItem
modifiedDateFormItem

安全性

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

// 微信服务号 生产配置中复写
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
    • private 是否采用下划线,应保持一致

bad case:

// 无意义命名
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 嵌套不多, 但是让人心智负担很重, 无法快速看出 count 值是多少会 false, 代码写的像面试题

if ((data && data.eid) || count++ > 20) {
  if (!data.eid) {
    webLog.custom({
      type: 1,
      code: 'getEidInfo-empty',
      msg: data,
    })
  }
  clearInterval(timer)
  resolve({ data })
}

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

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时看不出问题

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

代码审查方式对比

分类方式

审查方法

优点

缺点

审查方式

线下异步审查

时间灵活

实时性差

干扰小

易于存档

面对面审查

实时性好

对审查者干扰大

审查人数

一对一审查

安排容易

多人同时线下审查容易出现重复工作

干扰小

团队集体审查

讨论深入,审查细致

人数多时,容易效率低下

审查范围

增量审查

聚焦重点效率高


全量审查

系统性

工作量大,不能常常进行

专项集中检查


审查时机

代码入库前门禁检查

对于把关入库代码的质量,效果很好

太过死板的话,会降低代码入库效率

代码入库前的设计时检查

最早发现问题,从而大大降低问题修复成本;


代码作者抵触情绪小;


有效的架构讨论工具;


代码入库后检查

既不阻塞代码入库,又可以对提交的代码进行审查

有问题代码错过检查而上线的风险

担心冲突、害怕出错

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

冲突发生

  • 解决冲突
    ? Leader协助沟通及仲裁
    ? 协商达成共识
    ? 寻求第三人评估
    ? 组内讨论
    ?置若罔闻
    ?放任自由

预防冲突

  • 沟通技巧
    尽量疑向、不要太肯定
    ?如果采用......是否会更合适?
    ?这里应该......
    ?是否考虑过......这样的方案?
    ?......方案肯定更好
    ?这个地方似乎会影响滚动性能?
    ?这样写肯定会影响滚动性能
  • 发现问题,尽量提供建议
    ?......这样会更简洁
    ?你这代码复杂度太高了
    ?根据......项目规范,这里应该这样...
    ?你这代码不符合项目规范

特别注意

不要吝啬称赞
没必要力求完美

没有时间 CR

浇花很有意义, 但是先把火灭了

首先区分紧急与真正紧急:

不是真正紧急情况

真正的紧急情况

内部计划Deadline

显著影响用户生产环境的Bugfix

长时间开发后需要进行的必要合入日常Bugfix

导致整个团队工作暂停

回滚导致的测试失败或构建破坏

处理紧迫的法律问题


不跟版本会导致重大损失的Deadline


安全漏洞等

CR 时间不够

工作量评估要包含 CR 时间

推荐预留 20% 的 CR 时间

权衡:

关注设计大于具体实现;
保证不出线上问题为底线;

管理好交付里程碑

越大的里程碑越容易产生大型CL,会拖慢CR速度
建议数据:400行/小时(样式、dom行可适当剔除)
建议里程碑交付周期:1周,最长2周

真正紧急情况

同步CR

写完代码当面或电话同步Review

并行CR

结对编程

紧急情况后门

google 的做法:自己是Owner,写“To be reviewed”可绕过审查

历史包袱过重

通解: 卡住增量,治理存量
CR的目的是让每一次合并都在改善代码仓库的水平

其他-提升团队工程素养

? 设计模式: 掌握24种设计模式
? 设计原则: 掌握SOLID原则(单一职责、开闭原则、里氏替换、接口隔离、依赖反转)
? 方法学: 了解Devops,极限编程,Scrum,精益,看板,瀑布,结构化分析,结构化设计
? 实践: 实践测试驱动开发,面向对象设计,结构化编程,函数式编程,持续集成,结对编程

书籍推荐

《编程原则( understanding software)》
《重构:改善既有代码的设计》
《编写可读代码的艺术》
《代码大全》
《敏捷软件开发》
《架构整洁之道》
《代码整洁之道》

相关资源

Code Review 速查手册

相关推荐

5.5英寸触屏,搭载“安卓系统”的智能计算器评测:这设计挺脑洞

“计算器”可以说是我们日常生活中较为常用的一款电子产品,纵使手机上也有计算器功能,且足以替代实物计算器,但现在还是有很多人习惯用实物计算器,例如;做批发的店铺老板,计算器就放在店铺显眼位置,结账时顺手...

Android之父晒新款手机,造型酷似遥控器

安迪·鲁宾大家可能并不陌生,鲁宾曾一手创建了安卓操作系统,被外界誉为“安卓之父”。2015年鲁宾又创立智能手机公司EssentialProductsInc,还获得亚马逊和腾讯的投资。在筹备两年后,...

WP8.1的IE11为何不支持淘宝网触屏版?

IT之家(www.ithome.com):WP8.1的IE11为何不支持淘宝网触屏版?众所周知IE浏览器有自己的一个内核(简称IE内核),WP8.1系统的自带移动版IE11浏览器,但为什么iOS、An...

手机屏幕失灵乱跳乱点,屏幕时好时坏是怎么回事?

我们平时在使用手机的时候,如果我们的手机经常出现屏幕不受控制,手机屏幕会出现乱跳自己乱点的一些情况,这是什么问题呢?出现这种问题我们应该怎么去解决呢,今天我们九一手机维修就来跟大家说说这个问题该怎样去...

跨界表演有风险,百事手机 P1 众筹宣告失败

大家还记得曾经轰动一时的百事手机P1吗?这款10月份曝光、11月份众筹的手机在京东众筹失败,已经退款。想要喝着百事可乐玩着百事手机的网友,赶紧该干嘛干嘛去吧。据悉,百事可乐P1采用铝合金...

“傀儡”病毒感染超10万台手机

本报讯(记者孙奇茹)手机在没人操作的情况下,竟然自己亮屏、执行一些动作。这不是闹鬼,而是手机中毒了。日前,猎豹移动安全实验室发出警报,全球首个伪造模拟用户操作的安卓病毒被截获,并被命名为“傀儡(Go...

Android事件分发机制

事件分发机制Android事件分发是指在Android系统中,当用户触摸屏幕或执行其他操作时,系统如何将这些事件传递给正确的视图或组件进行处理的过程。Android事件分发遵循一种称为"事件分...

Android让视图像玻璃一样破裂

AndroidUILibs之BrokenView1.说明BrokenView让视图产生玻璃破裂的效果。注意:该库只能在API14以上的设备上运行2.配置在模块的build.gradle上面添加...

车载大屏爽翻了?英国研究机构:大尺寸触摸屏比酒驾更危险

大屏不仅蔓延到手机,汽车也不例外,得益于更加直观的人机交互体验,车载触控大屏逐渐成为越来越多车企的主流选择。然而最新的一项研究证明——触控大屏比酒驾、毒驾更危险。日前,英国一项道路安全研究报告指出,当...

安卓系统被曝严重安全漏洞 恶意程序竟能秘密拍照或录制音视频

央视网消息:据今日俄罗斯网站20号报道,以色列一家知名网络安全公司宣称发现谷歌、三星等制造商生产的安卓手机,系统存在严重安全漏洞,黑客能够在未经手机机主许可的情况下,操控安卓手机秘密拍摄照片、录制视...

央视曝光:安卓系统曝漏洞!有人可能正在用你的手机...

近日,谷歌、三星等制造商生产的安卓手机,被曝出系统存在严重安全漏洞。黑客能够在未经手机机主许可的情况下,操控安卓手机秘密拍摄照片、录制音视频并上传。点击下方,先看视频↓↓↓视频来源:央视新闻利用漏洞!...

安卓系统曝漏洞!有人可能正在用你的手机秘密拍照

近日,谷歌、三星等制造商生产的安卓手机,被曝出系统存在严重安全漏洞。黑客能够在未经手机机主许可的情况下,操控安卓手机秘密拍摄照片、录制音视频并上传。利用漏洞“恶意应用程序”拍照、录音视频并上传据今日俄...

安卓用户注意了 你的爱机可能会面临新式恶意攻击

据外媒报道,最近,东京早稻田大学的三名学者称,现代Android智能手机容易受到一种名为“Tap'nGhost”的新型攻击,这种攻击可以使不存在的“手指”点击手机屏幕,以采取行动。“Tap'nGh...

Android学习之Touch事件的处理

在移动开发过程当中,我们经常会遇到手势处理和事件触摸的情况,如果不了解整个事件的处理机制,对于开发的同学和码农是非常痛苦的,但是事件触摸的处理确实是一个非常复杂的过程,细讲起来,估计我都能讲迷糊,这里...

办公小技巧:制作模糊查询效果Excel下拉菜单

在Excel单元格中输入特定范围的内容时,一般会利用数据有效性生成下拉列表的方式进行,但如果源列表的内容太多(比方说有数百个),这时再通过下拉列表查找需要的值就很不方便了。如果采用智能感知的模糊查询下...