在 Code Review 要看什麼 講到了審查 CL 時該注意的事項，我們團隊也找到一個範例來練習，以下就是我們參考 MIT 的線上教學 所做的審查練習。
你在 GitHub 收到了一個這樣子的 PR（此範例以 GitHub 為主，因此 CL 直接改稱 PR）
變更內容如下
試著花些時間思考並回答下列問題：
- 你會如何去審查這段程式碼呢？
- 依照 Code Review 要看什麼 所講的要點，這段程式碼應該如何去審查？
- 依照 基本原則 所講的要點，這段程式碼應該如何審查？
審查練習
以下是整理後的團隊審查練習內容：
1. PR 內文與程式碼沒有提到這個變更的目的與功能
這個改動應放在你的 codebase 還是函式庫（library）裡？它跟系統的其他部分有沒有整合好？現在加入這些 CL 上的功能是好時機嗎？
— 審查要點一：設計
因 PR 內文僅提到『PR已做好了』並沒有辦法讓人知道這個 PR 的目的，自然無法回答要點內的這些問題。為了審查要點內的『設計』『複雜度』『功能』項目，審查者如果不清楚這個 PR 在幹嘛是無法進行的。
[審查建議]
PR 描述需要好好說明此 PR 的功能有什麼，想解決什麼問題，以方便審查者審查。2. 沒有寫測試
尤其在此 PR 內文沒有描述任何功能時，也沒有測試可以讓審查者了解功能規格，就更讓人不知道如何審查了。
[審查建議]
寫測試寫測試因為 PR 沒有定義功能規格，讓我們難以繼續審查，實際上 workshop 時團隊為理解這個功能也花了不少時間。為了順利進行，我們自行定義了此 PR 目的：
新增一個 utility function： 輸入年月日，輸出為此日期在當年的第幾天，例如：
輸入 1/31/2020 -> 輸出 31
輸入 2/28/2020 -> 輸出 59
有了功能規格之後才能繼續以下討論：
3. 有辦法使用原生語法或是第三方函式庫嗎？
因為此功能原生 Java 就有辦法達到一樣的目的，另外重新造輪子的目的是什麼？也許有什麼額外需求是沒想到的，這就會需要跟開發者就 PR 的目的來做溝通。
4. 依照對函式的定義，功能本身有許多錯誤
我們對此函式認知為：輸入年月日，輸出為此日期在當年的第幾天。但此 PR 會有兩個主要錯誤：
- 沒有考慮到閏年，二月永遠 28 天
- 12 月的數字是錯的 → 倒數第四行透過累加前十一個月總日數來算出 12 月的日期，但第 11 月是 31，應該要是 30
[審查建議]
沒有考慮到閏年，2020 年三月～十一月的 dayOfYear 都是錯誤的（少一天）
非閏年的 12 月也都是錯誤的（多一天）
請補上相關測試確保功能完整5. 過多 if-else 產生沒必要的閱讀複雜度
那一串 if-else 下面做的事情都類似，這情況下使用 if else 除了造成大量重複但相似的程式碼之外，還有可能產生閱讀上的困難。比如說 12 月輸出數字是錯誤的情況，在這樣大量且重複的 if-else 加上一堆相似數字下，是很難被發現的。這種情況其實可以透過迴圈、遞迴或是 Array reduce 簡化。
[審查建議]
每個 if-else 下面的邏輯都是一致的，只是依照月份需要累加不同的日期，這種情況其實可以透過迴圈、遞迴或是 Array reduce 簡化。6. Magic Number
雖然說 31, 30, 28 在幹嘛不算太難理解，但 31 + 28 + 31 + 30 + 31 + 30 + 31 呢？這種叫做魔術數字，雖然有意義但是一眼不容易理解的數字。
[審查建議]
避免使用魔術數字，若一定需要使用可以試著定義為常數
JANUARY_DAYS = 31
FEBRUARY_DAYS = 28 (先不考慮閏年)
MARCH_DAYS = 31在一些比較複雜的情境甚至月份也可以使用常數
JANUARY = 1
FEBRUARY = 2
然後 if-else 那段就可以改成 if(month == FEBRUARY) 之類
7. 無用的參數
在此案例的情況下其實 year 比較像是該用但是沒有用到的參數，如果把功能補完時的確會用到。
另一方面，無用的參數是滿常見的，這點可以直接寫明在樣式指南 coding guideline 裡面，甚至是進一步利用 IDE 來自動化處理。有時候會因為未來需求先把參數寫好，這點是不必要的，類似這種未來需求可以寫在功能規格說明書上，待實際開發時再來考慮。有時候會是舊的變數被刪掉，怕隨時要回復或參考過去邏輯而留下，這也是不必要的，git 是你的好朋友。
[審查建議]
year 沒有用到，有兩種可能，請視情況處理：
1. year 在計算閏二月時會需要使用，需要補上此功能
2. 如果 year 真的用不到，比如說，我們只會用在某一個非閏年的情況，那就把此變數拿掉8. 錯誤處理
參數型態為 int 代表了進來的參數是任何整數，而年月日都有自己的範圍，這點在函式中並沒有處理。然這牽扯到功能沒有定義清楚的問題，不知道這個函式會有誰來用，所以無法知道參數是否需要驗證。如果是大家都會使用這個函式那麼就會需要好好的驗證輸入參數，而如果只有一些固定地方會使用，並且傳入的值都會是正常的年月日，這種情況下的確是可以不用做參數驗證。
[審查建議]
需了解清楚函式會被用在哪裡，以決定是否需要驗證。9. 不必要的複雜度
[審查建議]
dayOfMonth 在函式中有兩種用途：一為傳入參數，二為儲存計算後的回傳值。一開始是傳入參數，然後經過計算之後變成一年內的第幾天，理解上會需要多一層轉換，多一層複雜度。可以參考 SRP: Single Resposibility Principle。
依照此原則應該把 dayOfMonth 拆分成 dayOfMonth(參數) 以及 days（計算後的結果）兩個變數，以增進可讀性。審查要點回顧
最後我們回頭看一下前文提到的審查要點，設計、功能、複雜度、命名、樣式、每一行、觀看角度在這個練習中都有被提到。沒有提到的有：
註解
此段程式碼雖然看起來不太直覺，但基本上是因為 PR 不清楚／ if-else 結構複雜，但本身邏輯是簡單的，如果可以照審查建議來處理之後其實不太需要註解。
文件
這是個小函式，因此也沒有相對應的文件需要維護，實際審查時就會需要檢查功能規格文件是否有相對應的調整。我們團隊的慣例是在 PR 內一定會附上相關功能規格文件的連結，這樣無論 QA 還是開發者來看此 PR 的時候都會有最正確的資訊。
好的部分
感謝 MIT 提供的這個範例，以及我們團隊成員 Larry 找到這個範例並且帶領大家跑一次 workshop。
討論與回饋
Medium 的文章回覆系統不是很好討論，如果對 Code Review 有任何想法或是想給我們回饋，歡迎到我們的粉絲頁貼文下方討論。
