Airbnb 資深工程師分享:怎樣才是正確、有效的 code review

20013034943_3034707e74_z

【我們為什麼挑選這篇文章】當開始與其他工程師合作專案時 code review 就變成了必做的功課之一,然而許多工程師大多走馬看花地瀏覽過,不然就是大刀闊斧地把自己不順眼的地方大修特修。

而本文作者朱贇,現為美國矽谷 Airbnb 的資深工程師,向大家介紹了幾個 code review 時應該注意的細節與架構。我認為不管對初出茅廬,或是身經百戰的工程師都有非常值得一看的價值。

(責任編輯:Matthew Chen)

之前寫過一篇《寫代碼的四個境界》,那個時候,大部分時候我還是愉快地寫著自己的代碼。Code review 也是每天工作的一部分,但是相對而言花的時間還是有限的。

最近一是因為角色轉換,二是突然來了很多新人。花在 code review 上的時間比寫代碼多出了好多,也有一些心得和感觸,隨便寫寫吧。

總的說來,矽谷稍具規模的公司 code review 的流程都是比較規範的。模式也差不多。一來所有的 PR(見下註解)都必須有至少一個人 stamp,才能 merge。如果改的東西涉及到多個項目,則需要每個項目都有人 stamp 才行。還有一些特別關鍵的代碼,比如支付相關的,通常也會需要支付組的人 stamp 才行。

NOTE: PR (pull request, a set of changes someone tries to push to a GitHub repository. Once a PR is sent, interested parties (peer engineers) can review the set of changes, discuss potential modifications, and even push follow-up commits if necessary.

在 stamp 前,通常是 github page 上可以給出各種 comments,page 是共享的,所以誰都能看到。有些 comments 是詢問,那麼代碼作者直接回复或解釋就行。有些 comments 是指出代碼的問題,這樣代碼作者可能會依此改動;也可能不同意,那就回覆 comments,有的時候關於一個實現細節,討論的 comments thread 可以多達十幾條。這樣對於大家達成共識很有幫助。

以前遇到有朋友說:

「看到代碼寫的太爛,覺得來回扯皮效率不高,乾脆撲上去自己寫。」

曾經我也有過類似的想法。不過最近遇到的一些事讓我慢慢地意識到這種想法很要不得。

首先就是從對方的角度來說。對方寫不好,可能是對業務不熟悉,可能是對語言不熟悉,也可能是對公司代碼的大架構不熟悉。如果你是幫他/她「寫」而不是耐心指出哪裡有問題,那麼下一次,他/她可能還是不知道。 不僅無益於別的人成長,有的時候甚至會讓別人有挫敗感

再然後就是這種方式的可擴展性很差。即使 code review 會花掉哪怕十倍於你自己寫的時間和精力,但它會讓人明白代碼應該怎麼寫,從長遠來看,這其實是在一定程度上「複製」你的生產力。你不能什麼都自己寫。尤其是你慢慢開始帶項目、帶新人。每天 review 五個人的代碼,和寫五個人的代碼,你覺得長期而言哪個更合算?

此外,如果說寫代碼是一個學習過程,怎麼做一個好的代碼審核人更是一個學習和成長的過程。自己繞過一個坑不難,難的是看到別人那麼走,遠遠地你就能告訴他/她那裡有個坑。而他/她在經你指出多次後,下一次他/她也會幫著指出別人的類似的問題。

這一點最近感觸尤為深刻。前一陣組裡咔咔咔接二連三來了好多新人,老大說,你少寫點代碼,多做點 review。於是那幾天我幾乎工作的一半時間都用來看代碼,寫 comments。可是最近就會發現,他/她們相互之間大部分時候已經可以很好的相互 review 了,於是我又騰出好一些時間來做別的工作。

Code review 做多了,發現其實也是有一定套路的,試著歸納如下:

  • 代碼格式方面

很多公司都有 coding style guideline。大家的約定俗成,避免公司的代碼風格不一致,也避免一些不不要的為了「要不要把閉括號另起一行」而無謂地爭論,除非是不小心,通常大家都不會弄錯。但是新員工往往會在這方面還不太熟悉。這一類問題也比較容易指出。

  • 代碼可讀性方面

這包括一個函數不要太長,太長就 break down。所有的變量名盡量能夠說明它的用意和類型。比如 hosting_address_hash,一看就知道是房東地址,而且是個 hash 類型。不要有嵌套太多層的條件語句或者循環語句。不要有一個太長的 boolean 判斷語句。如果一個函數,別人需要看你的長篇註釋才能明白,那這個函數就一定有重構的空間。另外,如果不可避免有一些註釋,則一定要保證註釋準確且與代碼完全一致。

  • 幫代碼作者想想他/她有沒有漏掉任何 corner case

很多時候這是業務邏輯相關的,尤其需要比較老的工程師幫助指出需要處理的所有情況。

  • Error handling

這是最常見也是代碼審核最容易幫別人看出的問題。舉個例子,下面一段簡單到不能再簡單的代碼就至少有三個潛在的問題:params 裡面需要 validate 是不是有 user_id 和 new_name 這兩個 key;能不能找到這個 user_id 對應的 user;save 的時候會不會有 DB level 的 exception,應該怎麼處理。

asdfg

  • 測試例和防坑

測試例不用說了,每段代碼都應該有測試例。但是對於一些你能預見如果別人改動代碼會引起可能問題的情況,一定要額外的加測試例防止這種事情的發生。這一點沒有例子參考也不太好說。怎麼寫好測試例,本身就值得寫一篇文章了。

  • 小架構

什麼意思呢,就是一個文件或者類內部的代碼組織。比如如果有重複的代碼段,就應該提取出來公用。不要在代碼裡隨意設常數,所有的常數都應該文件頂部統一定義。哪些應該是 private,等等

  • 大架構

這個就更廣了。包括文件組織,函數是不是應該抽像到 lib 或者 helper 文件裡;是不是應該使用繼承類;是不是和整個代碼庫的風格一致,等等。

當然,視具體情況可能還有很多別的需要在 code review 中註意的。但是如果按照這個 checklist 過一遍,一些明顯的問題就可以避免個八九不離十了。

另外,代碼 review 和寫代碼是我們每個工程師都應該致力的兩個方面,缺一不可。可能在工作中的某個階段或者某個項目裡,你會花更多的時間在某一面,但長久看來,兩個技能缺一不可。

  • 代碼審核是工作,不要抱有情緒化

我曾經幹過一件事,一個外組同事的代碼,別人已經 stamp 了,可是我覺得有問題,於是把 stamp revert 掉了,在 comments 裡寫了為什麼有問題,和建議的改法。當時心裡還有點覺得好像怪得罪人的。可是後來發現同事反而很客氣地接受並道謝了。心裡也是很感激有這樣的工作環境。

另外,經常會遇到有些 review 的 comments 裡出現不同意見,其實是兩個人在編程習慣和約定俗成上沒有共識。如果在不違背公司 style guideline 的情況下,沒必要一定讓對方和你有一樣的習慣。如果你真的覺得這樣更好,通常我也只會寫「I personally would prefer A over B, but no strong opinion.」或者「How about change it to A, since … However, this is not a merge blocker.」

人都有不周到的時候。多幾雙眼睛一起幫你看一遍,就可以很大程度地減少代碼裡的錯誤。另外, 相互 review 的過程中還能從彼此那裡學到很多編程的小技巧和好習慣 。細想來,很多 Java 和 Ruby 的特別好用的 feature,我都是在 code review 的過程中學到的。

——

(本文經合作夥伴 36kr 授權轉載,並同意 TechOrange 編寫導讀與修訂標題,原文標題為:《什麼才是 Code Review 的正確姿勢?》;圖片來源:Christiaan Colen, CC Licensed)

  • 延伸閱讀

【秘密武器?】傳 Google 研發新一代作業系統,要來取代市占率第一的 Android
騙鬼啊,台灣政府電子化全球 No.10!那為啥政府網頁還是用 IE?
特企》工程師的秘密基地:Github


及早將低碳節能的智慧用電納入經營策略,是企業維持永續競爭力的關鍵! 掌握台灣智慧能源管理創新趨勢,強化營運競爭力與成本控管能力 即刻報名》》智慧能源競爭力論壇

點關鍵字看更多相關文章: