說透程式碼評審

參考資料：

• 《網際網路大廠如何玩轉程式碼評審》 樑鬆華 京東高階開發工程師
• 《學習Facebook真正發揮程式碼審查的提效作用》 葛俊 前Facebook內部工具團隊Tech Lead
• 《程式碼審查哪種方式更適合我的團隊》 葛俊 前Facebook內部工具團隊Tech Lead
• 《聊一聊程式碼審查》熊燚（四火）Oracle首席軟體工程師
• 《程式碼審查普遍存在的 6 大問題》松花皮蛋me InfoQ
• 《程式碼評審：寄望與哀傷》胡峰 京東成都研究院技術專家

　　程式碼如熵，不加外力很容易就會隨著程式碼的不斷堆積而發生腐爛和潰敗。我們不是不知道程式碼問題，而是對既成事實難有改變。但是如果站在迭代的角度思考下一次升級，如何確保新的程式碼質量就顯得很有必要，特別是你的程式碼需要重寫的時候，我想你一定會遇到和我一樣的問題，我們究竟應該如何保證我們的程式碼的質量？於是就有了這篇工具型的文章。

        以下內容架構是我摘錄覺得比較重要的緯度，很多方法論借鑑了以上幾篇文章思想，對程式碼評審的各種情況基本都談得比較到位了，反覆精讀基本上就可以採用拿來主義，希望對你的團隊評審有所幫助。

為什麼要評審

不審查的壞處

　　程式碼的質量反應了我們的產品質量，產品不穩定、老是出現BUG，直接影響客戶滿意度和口碑。

　　同時，程式碼的好壞決定了未來運維的成本，如果因為一時疏忽和妥協，回頭又沒有及時修改，中間又出現人員變動，那麼這份程式碼的後患是無窮的。

　　因為不規範，可讀性差，對交接人來說從心態上是本能反抗的，但是又不得不改，於是就一通亂改，能貼膏藥就貼膏藥，能執行就可以，管他規範不規範。這樣導致的後果是，程式碼從不規範走向更加不規範，很難想象經過5-10年持之以恆的不規範，這個產品還能活著。

　　技術債務的危害怎麼形容都不為過，輕則系統區域性異常，中等的會導致修改困難，嚴重的需要推翻重來。

　　從物理學上看，熵讓我們理解了一件事，如果不施加外力影響，事物永遠向著更混亂的狀態發展，所以規範和審查就顯得彌足珍貴了。

　　從軟體設計看，軟體設計要關注長期變化，需要應對需求規模的膨脹。如果腐爛的程式碼日積月累，這些在不斷流變腐爛的東西又怎能支撐起長期的變化呢！

　　做產品，不審查不足以長久。

評審的好處

　　在軟體工程師日常的開發工作中，如果要挑出一項極其重要，卻又很容易被忽視的工作，在我看來程式碼審查幾乎是無可爭議的第一。程式碼審查是一個低投入、高產出的開發活動，就個人而言，從其中學到的習慣、方法和知識，獲益匪淺。

　　程式碼評審的作用很多，主要表現在 6 個方面。

　　好處 1，儘早發現 Bug 和設計中存在的問題。我們都知道，問題發現得越晚，修復的代價越大。程式碼審查把問題的發現儘量提前，自然會提高效能。

　　好處 2，提高個人工程能力。不言而喻，別人對你的程式碼提建議，自然能提高你的工程能力。事實上，僅僅因為知道自己的程式碼會被同事審查，你就會注意提高程式碼質量。

　　好處 3，團隊知識共享。一段程式碼入庫之後，就從個人的程式碼變成了團隊的程式碼。程式碼審查可以幫助其他開發者瞭解這些程式碼的設計思想、實現方式等。另外，程式碼審查中的討論記錄還可以作為參考文件，幫助他人理解程式碼、查詢問題。

　　好處 4，針對某個特定方面提高質量。一些比較專業的領域，比如安全、效能、UI 等，可以邀請專家進行專項審查。另外，一些核心程式碼或者高風險程式碼，也可以通過團隊集體審查的方式來保證質量。

　　好處 5，統一編碼風格。這，也是程式碼審查的一個常見功能，但最好能通過工具來實現自動化檢查。

　　好處 6，社會性功用。如果你在程式設計，而且知道一定會有同事將檢查你的程式碼，那麼你程式設計的姿勢和心態就會完全不同。這之間的微妙差異正是在於會不會有人將對你的程式碼做出反饋與評價。

評審的困境和爭議

　　大多數的爭議，都不是在否認程式碼審查的好處，而是聚焦在不進行程式碼審查的那些“原因” 或者 “藉口”上。

1）程式碼評審費時費力：

　　專案期限 Deadline 已定，時間緊迫，天天加班忙成狗了，誰還願意搞程式碼評審？這是一個最常見的客觀阻礙因素，因為 Deadline 很多時候都不是我們自己能確定和改變的。

改來改去無非是一些格式、註釋、命名之類不痛不癢的問題。

2）程式碼審查不利於團建：

　　因為經常有程式設計師因為觀點不同在程式碼審查的時候吵起來。

3）主觀意願

　　評審人用自己主觀意見看待別人的程式碼。覺得別人的程式碼風格不夠好，或者把個人的喜好強加到別人身上。

4）團隊人員結構搭配不合理。

　　新人沒經驗的多，有經驗的少。新人交叉評審可能效果不好，老是安排經驗多的少數人幫助 Review 多數新人的程式碼，新人或有收穫，但對高階或資深程式設計師又有多大裨益？

　　一個好的規則或制度，總是需要既符合多方參與者的個體利益又能滿足組織或團隊的共同利益，這樣的規則或制度才能更順暢、有效地實施和運轉。

5）有人就是不喜歡別人 Review 他的程式碼，他會感覺是在找茬。

　　比如，團隊中存在一些自信超強大的程式設計師，覺得自己寫的程式碼絕對沒問題，不需要別人來給他 Review。

6）效果非常差、形式主義

　　程式碼評審做得不好確實像形式主義，純粹走個過場。

搞清楚評審形式

　　落地之前，我們先搞清楚評審形式是什麼樣子的。

　　一般來說人工檢查才叫審查，機器進行的檢查一般叫作程式碼檢查或者程式碼自動檢查。常見的辦法是人工評審和機器檢查同時進行。

　　程式碼評審形式可以分為：

• 機器檢查
• 人工評審
• 純線上評審
• 線下投屏評審(推薦)

　　這裡推薦做法是線下投屏評審，或許傳統、保守，但是利大於弊。根據我的經驗，線上問題處理和後續工作計劃調整，所產生的一次性成本遠遠大於一次性線下評審。

評審物件有哪些

　　我們應該約定以下幾種操作都要進行評審，一般變動比較大的需要多人評審，一般性修改只需要其他同事二次檢查即可。

　　

評審參與人員

　　在這個問題上，原則是業務密切相關的人員都要參加，比如說：

1. 開發人員的導師
2. 負責這個業務流轉鏈路的下一個環節的同事

　　大家參與進來能夠及時同步資訊，避免資訊的不對稱，也就是說可以避免其他環節需要人員相容改動的，可是實際上沒有人考慮到，而導致漏改問題。

　　問題：為什麼說要堅持密切相關的人員都參加這個原則？

　　答案：大部分人對於自己不感興趣或者聽不懂的事務時，非常容易走神。所以組織業務密切相關的一線開發，組長，及領導參與評審就足夠了。

評審應該關注哪些

　　最後還有一個需要我們搞懂的問題，弄清楚之後，我們才能很好的把程式碼評審落地。

• 程式碼評審容易犯的錯誤：評審人用自己主觀意見看待別人的程式碼。

　　覺得別人的程式碼風格不夠好，或者把個人的喜好強加到別人身上，有這種想法固然好，如果覺得在團隊內應該統一使用這種風格，最好申請把他納入到團隊的程式碼規範文件中，以避免程式碼評審的重點產生偏移。

• 程式碼評審的目的：最大化維護團隊的利益。

　　為了軟體的良好發展和團隊的高效協作，每個人的程式碼最好看上去都差不多，這樣修改別人的程式碼就比較親切。

• 程式碼評審應該關注的重點：
• 是否明顯的邏輯錯誤
• 是否落實了程式碼規範
• 程式碼的可讀性和可維護性是否良好
• 程式碼是否有違背基本的設計模式理念
• 程式碼評審不應該過多關注：
• 程式碼是否能正常執行
• 程式碼是否滿足業務需求
• 是否覆蓋業務場景

　　這些應該由編寫程式碼的人和測試人員共同保證

評審的流程有哪些

　　每個想要高效工作的程式設計師，都不希望自己的計劃被頻繁打斷，大多數情況下按部就班，跟著規劃走才是最穩妥的。

　　下面是評審中的六個流程：

　　

• 約定規範文件

　　為了避免審查當中的主觀喜好，制定有章可循的約定是前提條件。

• 制定排期：

　　我們可以約定每週一發起程式碼評審，由提交人根據邏輯變動情況，給出一個評審大概需要花費的時間，同時結合需求的提測時間，上線時間，確定好什麼時候進行評審，避免過多雜亂的時間線。

• 補充資料

　　在評審工期申請後，還需要補充資料，交代清楚需求背景、編碼內容以及功能影響的範圍，評審人就可以根據這些資料，評估應該重點關注哪些程式碼隱患。

比如需求涉及到金錢往來，那麼評審人就應該更加仔細留意每一行程式碼，檢查是否有明顯的邏輯錯誤，避免上線後導致使用者或者公司的利益受到損害。

• 程式碼評審

　　程式碼評審中發現的問題，根據嚴重性決定是否進行二次評審。

• 完善必備要素

　　發現問題並不可怕，可怕的是相同的問題不同的同事重複犯，所以必須在評審後完善程式碼評審必須具備的元素清單，比如監控、註釋等。如果不滿足任何一個，直接駁回，畢竟重複提及相同的問題，會大家參與者的積極性。

• 完善程式碼規範約定

　　另外也需要定期回顧之前的程式碼評審，完善團隊程式碼規範約束，讓軟體質量和團隊能力都想著更好的方向走。

審查步驟和方法

　　從我的經驗來看，要成功引入程式碼審查，首先要在團隊內達成一些重要的共識，然後選擇試點團隊實行，最後選擇合適的工具和流程。

　　個人覺得不是所有的團隊都適合做程式碼審查，一定要慎用。特別是那種專案型別的團隊，救火型別的團隊，團隊規模很小，業務優先的團隊。

1、程式碼審查應該計入工作量

　　程式碼審查如果沒有為它預留時間，結果是大家沒有時間做審查，效果自然也就不好。於是，形成惡性迴圈，程式碼審查要麼被逐漸廢棄，要麼流於形式。

　　預估工作量的時候需要考慮程式碼審查的時間。比如，平均每天大約預留 30-60分鐘用於程式碼審查，大概佔寫程式碼總時間的 1/5。

　　一般的程式碼審查速度約是一小時 150 行，對於一些關鍵的軟體，一小時數百行程式碼的審查速度太快，可能無法找到程式中的問題。

　　同時，程式碼審查的情況會作為績效考評的一個重要指標。

　　另外，平時需要給審查者關於審查質量的實時反饋。比如，剛加入公司的時候，對程式碼審查不夠重視，做得不夠好。主管應該對給反饋意見，讓新人提高審查質量。

　　其次，讓新人多給同事做審查，另外，是讓新人多給一些結構上的建議，不用太重視語法細節。

　　總之，管理者要明確程式碼審查是開發工作的重要組成部分，並記入工作量和績效考評。這，是成功引入程式碼審查的前提，再怎麼強調都不為過。

2、選擇試點團隊

　　為什麼要選擇試點團隊，說白了就是要避免紙上談兵的不足，在小範圍內做實驗可以有效降低風險，儘可能得收集負面效果，並及時改進。否則大規模出現負面效果是很影響大家信心，甚至出現反面效果。

3、選擇工具，融合機器審查和人工審查

　　關於工具，如果你的團隊本來已經在使用 GitLab、GitHub、Gerrit、Phabricator 管理程式碼的話，那麼很容易上手程式碼審查。因為，GitHub、GitLab 有基於 PR 的審查。而 Gerrit 和 Phabricator 本身就主打程式碼審查的功能。

　　第一，將程式碼提交到本地 Git 倉庫或者用於審查的遠端 Git 伺服器的分支上；第二，把 commit 提交給程式碼審查工具；第三，程式碼審查工具開始進行機器審查和人工審查；第四，如果審查通不過就打回重做，開發者修改後重新提交審查，直到審查通過後程式碼入庫。

　　

　　關於工具集的適用：使用 GitLab、Jenkins 和 SonarQube 進行配置。具體使用 GitLab 管理程式碼，程式碼入庫後通過鉤子觸發 Jenkins，Jenkins 從 GitLab 獲取程式碼，執行構建，並通過 Sonar 分析結果。這裡有一篇不錯的文章供你參考

評審的關鍵操作

• 提高提交的原子性

　　每次提交的程式碼粒度至關重要，我們可以反過來思考：

• • 如果提交的是半個功能的程式碼會怎麼樣？
  • 如果提交的是一週的程式碼會怎麼樣？

所以原子性就是合適的粒度，大功能要拆分來提交，一週的程式碼的程式碼要按天來提交。否則對於評審人員來說是很反感的，因為只會增加審查的難度。

• 提高提交說明的質量

　　我們應該經常見到很多的提交是這樣描述的：

• • BUG
  • FIX
  • 更新

　　下面是葛俊給出的三個改進步驟：

　　第一步，規定提交說明一定要包括標題、描述和測試情況三部分，但暫時還不具體要求必須寫多少字。比如，測試部分可以簡單寫一句“沒有做測試”，但一定要寫。如果格式不符合要求，審查者就直接打回。這個格式要求工作量很小，比較容易做到，兩個星期後整個團隊就習慣了。雖然只是在提交說明裡增加了簡單描述，也已經為審查者和後續工作中進行問題排查提供一些必要資訊，所以大家也比較認可這個操作。

　　第二步，要求提交說明必須詳細寫明測試情況。如果沒有做測試一定要寫出具體理由，否則會被直接打回。這樣做，不但為審查者提供了方便，還促進了開發人員的自測。整個團隊在一個多月後，也養成了詳細描述測試情況的習慣。

　　第三步，逐步要求提交的原子性。我要求每一個提交要在詳細描述部分描述具體實現了哪些功能。如果一個提交同時實現了多個功能，那就必須解釋為什麼不能拆開提交；如果解釋不合理的話，提交直接被打回。

　　提交說明是提高程式碼審查的利器，好的格式應該包含以下幾個方面：

　　標題，簡明扼要地描述這個提交。這部分最好在 70 個字元之內，以確保在單行顯示的時候能夠顯示完整。比如，在命令列常用的 git log --oneline 輸出結果要能顯示完全。

　　詳細描述，包括提交的目的、選擇這個方法的原因，以及實現細節的總結性描述。這三個方面的內容最能幫助審查者閱讀程式碼。

　　測試情況，描述的是你對這個提交做了什麼樣的測試驗證，具體包括正常情況的輸出、錯誤情況的輸出，以及效能、安全等方面的專項測試結果。這部分內容，可以增加審查者對提交程式碼的瞭解程度以及信心。

　　與其他工具和系統相關的資訊，比如相關任務 ID、相關的衝刺（sprint，也可翻譯為“迭代”）連結。這些資訊對工具的網狀互聯提供基礎資訊，非常重要。這裡還有一個 Git 的技巧是，你可以使用 Git 的提交說明模板（Commit Message Template），來幫助團隊使用統一的格式。

評審的關鍵原則

　　雖說評審需要規範文件做指導，但是很多規範其實無法做的那麼細，特別是規範早期，很多規範是缺失的。在評審過程中難免會出現爭論或者相持不下的情況。這個時候有必要把評審的原則提前做一個說明，可以消除未來不必要的麻煩。

• 相互尊重

　　從編碼作者的角度來看，審查者要花費時間去閱讀他並不熟悉的程式碼，來幫助你提高，應該儘量為審查者提供方便。

• 比如，提高提交說明的質量，就是對審查者最基本的尊重。
• 還有，如果你的程式碼都沒有進行自測就提交審查，你覺得審查者心裡會怎麼想呢？
• 又如，如果你提交的一個審查有一萬行程式碼，讓審查者怎麼看呢？

　　所以，程式碼作者一定要提審查者著想，幫助審查者能夠比較輕鬆地完成審查。

　　從審查者的角度來看，在提出建議的時候，一定要考慮程式碼作者的感受。最重要的一點是，不要用一些主觀標準來評判別人的程式碼。

• 基於討論

　　程式碼審查的目的是討論，而不是評判，這個原則至關重要。

　　討論的心態，有助於放下不必要的自尊心，從而順利地進行技術交流，提高審查效率。

　　另外，討論的心態也能促進大家提早發出審查，從而儘早發現結構設計方面的問題。

　　另外，我還有一些關於討論的建議：審查者切記不要說教，說教容易讓人反感，不是討論的好方法。

　　審查者提意見即可，不一定要提供解決方法。給定答案也可能會引起不必要的反感。

程式碼評審的常見問題

　　這些是程式碼評審過程中發現的共同的問題，可以一起放在程式碼規範文件中。

缺少註釋和變更說明

　　比如下面三個方法名稱，引數，返回值相似，為什麼會出現這種情況？

　　大半是開發者發現原來的程式碼沒有註釋，所以不敢修改，於是拷貝一份後只是稍微修改了一下，這樣才出現了相似冗餘的程式碼。重複程式碼最可怕的地方就是錯該或者漏改。

publi Article GetArticleById(long id)
{
  return null;
}
publi MaterialPO GetMaterialById(long id)
{
  return null;
}
publi ArticleVO GetArticleByIdWithoutStatus(long id)
{
  return null;
}

過渡相信第三方

• 對請求引數沒有限制
• 對請求引數沒有校驗

//錯誤示範
Boolean SaveError(List<ShareDetail> list)
{
   //錯誤原因：只判斷非空，但是無法保證list中的detail是非空的
   if(list.IsEmpty())
   {
      return false;
   }  
   //錯誤原因：無法保證list中的id都是一樣的
   Int id = list.get(0).getId();
   //儲存資料
   return Save(id,list)
}


//正確示範
Boolean SaveRight(Int id,List<ShareDetail> list)
{
   //深度判斷非空情況
   if(id==null || CollectionUtil.hasNull(list))
   {
      return false;
   }  
   //id從引數中獲取，避免二義性
   return Save(id,list)
}

變數作用域過大

public static void main(Sstring[] args){
  ShareDetail shareDetail1 =new ShareDetail();
  //錯誤示例：物件在多個方法間進行值地址引用傳遞
  varErrorStep1(shareDetail1);  
  //正常示例
  ShareDetail shareDetail2=varRight();
}


public static varErrorStep1(ShareDetail shareDetail)
{
   //其他複雜操作
  shareDetail.setId(111);
  var ErrorStep2(shareDetail);
}


public static void varErrorStep2(ShareDetail shareDetail)
{
  //其他複製操作
  shareDetail.setName("hello");
}


public static ShareDetail varRight()
{
  ShareDetail shareDetail =new ShareDetail();
  shareDetail.setId(111);
  shareDetail.setName("hello");

缺少階段性結果

　　對於過多的if-else判斷，優化方法：先進性異常判斷，如果有異常就快速返回結果。

//錯誤示範
public void SetpError()
{
  //標記位
  Boolean flag=false;
  List<Integer> list=new ArrayList<>();
  for(Integer detail:list)
  {
    //如果列表中有值大於10則將標記設定為true
    if(detail>0)
      {
        flag=true;
      }
    }
  }
  //
  if(flag)
  {
    return；
  }
}
//正確示範
public void SetpRight()
{
  //步驟一：校驗引數
  List<Integer> list=new ArrayList<>();
  for(Integer detail:list)
  {
    //如果列表中有值大於10則將標記設定為true
    if(detail>0)
      {
        return; //移除多餘的標記，直接中斷；
      }
    }
  }
  //步驟二：
  
  //步驟三：


}

　　同時為了增加程式碼的層次感，可以讓程式碼階段性的輸出結果，比如編寫時註釋好每一個步驟要做什麼。

日誌列印問題

//錯誤示範
public Boolean logError(Integer id,List<ShareDetail> list)
{
  if(id==null || CollectionUtil.hasNull(list))
  {
      return false;
  }
  logger.info("logError run");
  
  try{
    return shareDao.save(id,list);
  }catch(Exception e)
  {
    //錯誤一：不記錄異常上下文
   logger.error("save error");
   //錯誤二：只記錄了有限的上下文
   logger.error("save error e:{0}",e.getMessage());
  }
  return false;
}


//正確示範
public Boolean logError(Integer id,List<ShareDetail> list)
{
  if(id==null || CollectionUtil.hasNull(list))
  {
      return false;
  }
  //info級別在非線上環境很容易
  logger.info("logError run");
  
  try{
    return shareDao.save(id,list);
  }catch(Exception e)
  {
    //正確：充分記錄錯誤上下文
    logger.error("save error");
    //正確：規避記錄日誌時出現控制著，規避記憶體OOM
    logger.error("save error id:{},e:{},list:{},id,list==null?list:JSONObject.toJSON(list),e);
  }
  return false;
}

總結

　　想讓團隊從心裡接受程式碼評審的理念，認可它是日常開發過程必不可少的步驟，那麼就要提高程式碼評審的質量，否則評審的時候，大家都在搞私事，容易流於形式，而要提高程式碼評審的質量，我們必須明確程式碼評審的形式，物件，參與者，關注點，流程，常見問題。

　　我希望大家能記住一件事，程式碼評審要從團隊利益出發，讓每個人都能夠從裡面持續得到正反饋，這才是最重要的。如果時間充足的話最好邀請測試人員一起參與，讓測試人員充分了解程式碼改動點，有助於對測試場景邊界的把控。