前言

隨著現在開發軟體、韌體的人數增加,大家協作同一份 repo 的頻率開始上升,尤其實在公司決定採用 Monorepo 架構後,協作頻率更是呈現指數型狂飆阿。

工程師難免會因為 deadline 壓力,而遞交出臭臭髒髒的 code,為了避免改 A 壞 B 這種情況發生,以及日後的維護性。這時,如何把關程式碼品質,又不影響到開發進度,就是 Reviewer 重要的課題。

我之前沒有 Code Review 經驗,想藉由整理這篇文章,好好學習學習。

What is Code Review ?

Wiki 上的定義

Code review is a software quality assurance activity in which one or more people check a program, mainly by viewing and reading parts of its source code, either after implementation or as an interruption of implementation. At least one of the persons must not have authored the code. The persons performing the checking, excluding the author, are called “reviewers”

它的目的是某種程度上預防可能的錯誤,和增進程式的品質。 而除此之外,透過適當的 code review 更可以讓團隊裡面的每一位成員了解各個部份的程式碼。 如此一來,在維護同一份程式的時候,團隊的成員就更能對程式的架構有一定的共識。可以避免不小心重覆做了其實已經有的功能,或者是做了一個功能結果發現和本來設計的理念大相逕庭的狀況發生。

Code Review 看什麼

1. Design

是審查中最重要的部分,看這次的 CL的內容是否合理,是否會跟其他系統產生衝突? 現在加入這項功能是好時機嗎?

2. Functionality

CL 有達到開發者的預期嗎? CL 有幫助到 user 嗎? (公司其他的工程師或是客戶)

通常,我是說”通常”,我們會期待開發者自己把 CL 做好完整的測試。但,只要是人都會犯錯,所以需要 Reviewer 來幫忙一起檢查,是否有考慮到 edge case、concurrency 問題。嘗試站在使用者角度來看這份 CL。

尤其是在 CL 是關於 “使用者感受的到” 的部分,例如 UI 的更改。這時候,光看 code 很難知道這些改動是如何影響到使用者。對於這一類的CL,可以要求開發者附上 demo,例如 UI 改動前後的截圖等。

另外,要特別注意的是有沒有因為 parallel programming 而造成 deadlocks or race conditions。這類的問題,通常不容易發現。最輕鬆的解決方式,就是不去使用這一類的 Lib or model。(逃避可恥,但是有用)

3. Complexity

CL 是否太過複雜? 檢查每一層級,單行code是否太複雜? function 是否太複雜? class 是否太複雜? 太複雜,代表不易讀,也代表其他工程師要使用或修改時,容易發生錯誤。

另一種太複雜狀況是 過度設計,開發者把 code 變得太過通用,或是加入了現在不需要的功能。雖然開發者是求好心切,為未來可能需要解決的問題做準備,但 Reviewer 還是需要注意此狀況,讓開發者專注在現在要解決的問題上。

4. Tests

根據情況,要求 unit, integration, or end-to-end tests,我知道很多工程師都不喜歡寫測試,這時候 Reviewer 需要硬起來要求!

記得,測試也是需要被維護的,不要因為它不是主要功能,就忽視它。

雖然寫測試不是仙丹,能治好百 bug,至少能在一定程度上把關程式碼品質。如果把程式碼比喻成一台火車,那麼,寫測試至少可以確保,這一台火車不會開一開,半路上突然開出軌。

延伸閱讀:

5. Naming

正確的命名,能夠大幅增加程式碼可讀性。

取名取什麼那 a,b,c 都欠吊起來打! 拜託不要互相傷害。

6. Comments

解釋程式碼為什麼存在 (Why),而不是解釋程式碼做了什麼(What)。
如果程式碼不能輕易被理解,那應該要想辦法簡化程式碼。

但還是有些例外,例如像是一些複雜的演算法,或是商業邏輯。

延伸閱讀:

7. Style

就看看公司有沒有規定囉。如果沒有,可以參考 Google

延伸閱讀:

  • [Python Coding Style]

8. Consistency

如果現存的程式碼不符合 style 怎麼辦? 規定就是拿來遵守,改就對了,讓程式碼品質就像童子軍規則一樣 (離開營地前,讓營地比使用前更加乾淨)。每次打開專案,我們就改一點,讓程式碼的品質變得更好。

9. Documentation


如果 CL 更改的介面會影響到其他人使用的地方,請檢查相關文件是有一併做更新。

10. Every Line

通常,會需要看過每一行 CL 的程式碼。像一些 data files, 自動生成程式碼等,可以快速掃過。

但只是要人寫出來的程式碼,請仔細慎查,確保你知道這些程式碼的用途。

如果 Reviewer 覺得程式碼讀起來很痛苦,很難讀,讓整體 review 速度變慢。那麼,應該要讓開發者知道這一點,讓他們把 CL 修改成更容易懂,再遞交審查。這也幫助之後維護這份程式碼的工程師。

如果你覺得不夠資格審查某段程式碼,請找一位夠格的審查者。

11. Context

用不同的角度審查CL。有時候,雖然只是改動幾行code,必須看整份檔案才能確保改動是合理的。

例如: 改動只有4行 code,但他把這4行加到一個100行的 function,當你看到整份檔案,你就會發現這 function 需要被拆解。

12. Good Things

如果在 CL 看到不錯的內容,告訴開發者,有時告訴開發者他們做對了什麼比告訴他們做錯了什麼更有價值。

Summary

Review 時,確認以下幾點

  • 程式碼都是經過精心設計的
  • 該功能對於程式碼的使用者來說是有用的
  • 任何 UI 改動都要是合理的
  • 任何平行運算都要安全地完成
  • 程式碼沒有過度設計
  • 開發者並沒有實作將來可能需要,但不知道他們現在需不需要的功能
  • 有適當的單元測試
  • 測試有經過良好的設計
  • 名稱是有意義的
  • 註解是清楚且有用,解釋 why 而非 what
  • 程式碼有適當的文件記錄
  • 程式碼有符合 coding style