程式碼質量與技術債系列分享之一 - 如何做好 Code Review

京东云开发者發表於2024-03-29
View

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意義

靈魂拷問：為什麼我們接手的每個程式碼庫都如此難以維護？

image.png
重要原因之一：Code Review 執行不徹底
萬能藉口：太忙！

  • 疲於應付眼前

  • 不可見，意識不到

  • CR 非功能性開發

  • CR 不是當務之急，沒有眼前收益

  • 危害被低估，忽視“複利”的威力（非線性）

意義

現代程式碼評審【modern Code Review】，業內認為有效的、基於最佳實踐的質量保證工作流，可透過人工審程式碼降低風險、增強可維護性和提升研發效率，同時可以有效提升個人和團隊技術能力。更是一種對程式碼精益求精、追求極致的態度、是“工匠精神”的一種體現。

CR原則

  • 只要CL可以提高整體程式碼健康狀態，就應該傾向於批准合入，即使CL並不完美

  • 基於技術事實和資料的溝通

    • 基於技術事實和資料否決個人偏好和意見

    • 軟體設計問題不能簡單歸結為個人偏好

  • 解決衝突：不要因為無法達成一致而卡殼

  • 善用工具

    • 基於Lint、公司程式碼規範等工具

    • 大模型輔助

發起CR

發起前的準備

  • 推薦自己做一個 checklist

  • 把自己當作 Reviewer 來對自己的程式碼進行 CR

  • 預估程式碼可能出問題的地方

  • 進行充分自測，保證程式碼可執行

  • 不要指望別人幫你找出問題

checklist 可參考Code Review 速查手冊

利用自動化工具進行前置檢查

  • 單元測試檢查

  • 新增單元測試檢查

  • 方法行數過多

  • 圈複雜度過高

  • 程式碼規範檢查

  • lint 檢查

  • 體積監控

建議平均時長不要超過10分鐘， 所以 e2e，效能檢查等建議不阻塞發起MR流程

合理的規模

image.png
image.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：
image.png
“修復錯誤”是不充分的 CL 描述。什麼 bug？你做了什麼來修復它？

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

  • 邏輯修復

  • 新增補丁

  • 增加XX規則

  • 刪除XX邏輯

Good Case：
◆ 摘要：【xx模組】新增xx功能
◆ 背景： 新功能需求，要求xxx， 詳見[卡片連結]
◆ 說明： 由於xx，新增xx處理模組…

  • 摘要：刪除 RPC 伺服器訊息自由列表的大小限制

  • 說明：像 FizzBuzz 這樣的伺服器有非常大的訊息，可以從重用中受益。使自由列表更大，並新增一個 goroutine 隨著時間的推移慢慢釋放自由列表條目，以便空閒伺服器最終釋放所有自由列表條目。

必要時，應使用 cz 等工具進行規範。

心態

  • 一次 CR 其實是開啟的一次“對話”

  • 應該期待評論和反饋，並及時進行回覆

  • 做好心理準備自己的程式碼可能會有缺陷

  • CR 的目的之一就是發現問題， 所以不要有牴觸

CR內容

程式碼是寫給人看的, 不是寫給機器看的,只是順便計算機可以執行而已。------《計算機程式的構造和解釋》

應該被 CR 的內容：

自上而下，優先順序從高到低：
程式碼質量與技術債系列分享之一 - 如何做好 Code Review

模組簡介
設計 程式碼是否設計良好並適合您的系統？
功能 程式碼的行為是否符合作者的預期？程式碼的行為方式對其使用者有好處？
安全性 程式碼是否安全合規？
複雜性 程式碼可以更簡單嗎？當其他開發人員將來遇到此程式碼時，他們是否能夠輕鬆理解和使用此程式碼？
測試 程式碼是否具有正確且設計良好的自動化測試？
命名 開發人員是否為變數、類、方法等選擇了明確的名稱？
註釋 註釋是否清晰有用？
風格 程式碼是否遵循京東程式碼風格規範？
文件 開發人員是否更新了相關文件？

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

CR流程順序

image.png
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 ? (
    <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 trueisNotOnlineInvoice 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多行程式碼就成了這個工程的毒瘤。

image.png

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 速查手冊

相關文章