當 Senior Python 工程師的 Pull-Request 被 Code Review (程式碼審查)

網路上不難查到為何專業軟體開發要做 code review、它的效益與流程，這裡主要會聊聊 Senior 工程師在做 code review 或發出 pull-request（以下也會用 PR 簡稱）時，藉過往經驗與 Python 的例子，整理了一些關於軟體工程品質的要點。

Disclaimer：程式寫得好，在不同的工作目標與組織文化環境下，可能與你的職級與加薪無關。那與什麼有關呢？想要 TL;DR 的話可跳到最下方直接讀總結，後有彩蛋。

本文內容應該是 PR 作者與審查者都可以注意的，我們就從一些基礎的點開始吧。

Day 0：工欲善其事，必先利其器

找或架一個有 CI（Continuous Integration）幫忙 code review 的環境（例如來 iCHEF 一起玩），人生有限，快省下花在討論 flake8、isort 或 pre-commit 能做的 PEP 8 檢查的心力吧！還能 black 就更完美了。

• 糾結於幾十年人生裡的某一個份工作裡的某幾行被自動排得漂不漂亮，也是一種浪漫？🌹

簡單比複雜好

The Zen of Python（python -m this）裡可說是它整個主旨的那句

"Simple is better than complex."

因為簡單，所以好讀；因為簡單，所以直白、平順、明確而美麗。

延伸到 pull-request 的話，可以這麼看：

1. 小的 PR 比大的 PR 好，切得適當的 PR 可能比切成十個 PR 值得。
   同事說：對 reviewer 好一點，不然 reviewer 就得兇一點了。
2. 愈早的 PR 比愈晚的 PR 好，前兩三個 PR 先丟出來比累積五個 PR 好討論，自己也好修改。
3. 乾淨的 PR 比混雜的 PR 好。
   反例：嘿這邊有個 2000 行 diff 內含 4 個 design pattern 還有 3 個多重繼承的 class，厲害吧。❌

有禮貌比沒禮貌好？

這樣說可能有點過頭了（笑）。在 iCHEF 實戰開發時，我們很鼓勵發了 PR 後自己再讀一遍：自己寫的都覺得不好讀了，別人讀起來應該更痛苦。

• ⭐️ 小技巧：在 PR review 裡留個自己的小 comment。
  讓 reviewer 知道這不是自己沒看過就丟出來請人幫看。

logging 好像接近禮貌的等級了：改資料時加個有意義 log。例如

2022-12-16 21:31:29 INFO user id:2 ;; age updated from 3 to 18

而非

2022-12-16 21:31:29 INFO user changed  # 要追改了什麼、發生什麼事會很無力。

logging 如此重要，我們甚至可以為了好寫、好在 AWS CloudWatch 上追 log，寫了一些自用的 helper utils。

PR 常見的兩種挑戰：規格與非規格

非規格也可以說是「技術問題」，如單純在程式、測試的寫法上的問題。
📋 例如在純讀取 API（常見如 HTTP GET）裡去做資料的修改，就屬此類。

而規格的實作是否精確，很依靠對整個專案規格的理解；假如只關心自己實作的部份，在專案中後期整合時發現實作不合預期的風險就更高了。

也有兩者都有關的情境。📋 例如為規格新增欄位時，是否想到要確保新舊資料的情境，或當前後版的 app 同時存在時仍可相容，避免上線後才發現這類很容易發生的非預期行為。📋 另外需要一次性全面 DB migration 時，上版時是否可能因此造成倒站（downtime）也是常見的考量點。

PR 成敗決定在寫 code 之前

常見專案時程大失準的前因：沒有任何計畫的專案估時，加上專案開工寫 code 時才開始的設計。

而預期外的工程 delay 所產生的心理壓力，也容易引發一連串 🧨 災難型的 PR，然後團隊士氣低落、人員離職，需要交接、找新人與培訓，又產生預期外的工程 delay，如此惡性循環。

除了止血點總是很重要之外，想避免那樣的情況，可參考文未所附的 Design Review（另一種說法是技術審查 Technical Review）。這邊補充同時可搭配的手法與心法：

DFD by Am_I_Helpful

1. 預期有複雜的流程或狀態時，先畫一些資料流，更容易在早期發現預期外的工程。例如常用（或可說最有用）的幾個圖 🎨：UML 循序圖（Sequence Diagram）、狀態圖（State Machine Diagram）、ER Diagram，以及 DFD（Data Flow Diagram）
   
   📋 案例：我們以為下訂單時都會有結帳資訊，在專案執行到中後期時發現：當下的實作是不會有結帳資訊的！後來雖花了額外的心力解決了，想像當初在這邊畫 DFD 的話，也許有機會在更早期發現問題並與團隊討論相關的規格修改或預期的工作。💭
2. 用最短時間先把 code 從頭到尾走過、順一遍，確定最後產品出去會動。有時遇到 Design Review 裡有自己比較不確定的技術細節時，會蠻需要靠這招。
   ● 畢竟說到底開發到最後跟老闆說不能上線，也相對難以被接受。同時這也會大幅提升自己對專案裡程式架構的掌握，以及對時程的基本預期。
3. 不要因為麻煩而走捷徑，而是思考怎麼樣的實作才符合工程邏輯與原則。📋 例如：下訂單時有第三方的 payment 失敗時要更新 order 狀態的地方，想像中要盤過的地方好像很多，不如就在讀取 order 的時候去假裝狀態或再來改狀態。
   
   這邊有兩個明顯的代價：
   
   a. 後人在追 code 時的直覺會去找 order 更新狀態的地方，找到最後才發現，原來是在讀的時候才即時算出狀態，那其它的狀態更新是不是也可能在讀的時候又被改掉？這也影響了程式碼與資料流的可信度與可靠度。
   
   b. 💣 如同 HTTP GET 的時候偷偷去改資料，在 GraphQL 用 query 時去改資料也是同樣的意味，過往經驗都是個（必爆的）未爆彈。
   ⭐️ 最後仔細盤過並 refactor 的程式，遠比預期中小而容易修改，同時也讓整體程式的流程更加清析而容易使用、查詢、理解了。
4. 當然，很多時候能不用寫 code 發 PR 最好。原則是為了必要而寫，能不用非同步、不用加 queue 就不用，能用 Factory、不寫 generate function 就不用，能用簡單的業務規則機制防守，就不用複雜的各端工程防守。
   📋 例如在每個 function 裡都寫 assert user_id 不是很多餘嗎？整個 code base 都是這個的話是不是很混亂、容易錯失真正要好好防守之處？也許在某一層就能守的剛剛好。
   
   能判斷、溝通 🗣️ 必要、應該要，以及多餘的部份，也是蠻 senior 的點。
5. 測試即 spec。也是 iCHEF 後端近 95% Coverage 與近 9000 個測試的原因：即然程式是為了產品規格而寫的，PR 裡為「說好的規格」寫下對應的測試也是很合理的，後人（常見是半年後的自己）也能從測試感受到當初的情境，並更容易推測或看出當初的 spec。
   
   👉 Code review 時，看到規格改了、程式改了，測試卻沒什麼變動？就可以特別想一下，是不是原來的測試沒測到預期的規格，或著程式沒改好？
   
   ⭐️ 小技巧：也可以想一下，拿掉測試的先決條件或改動的程式碼時，測試真的會壞掉嗎？通常我們也建議反向測試至少 local 試過並標註於 PR 中，一種簡易 TDD（Test-driven development）的概念。

同事或半年後的自己，看到測試與程式裡的註解，都能有更深的把握。修起 bug 也比較不會那麼惋嘆。

讓你 PR 裡的 Python 程式碼更簡明的小技巧

好程式常有的特徵：簡單直白、一目了然、或想了一輪回來看，它就是各種考量後的最佳解。

那也不一定是簡單的函式，有困難的部份它會試著跟你溝通，告訴你這邊為何不簡單、當初的情境是什麼。畢竟工程的最佳解會受限於人事與業務邏輯上的複雜度。

1. PR 裡的描述註解 vs 程式註解（code comment）：需要在 PR 裡再補充的，常常 comment 到程式裡更好，因為後人先讀到的通常是 code，要找到當初的 PR 相對困難而遙遠，也可能找不到（例如有天想從 GitHub 換到 Gitlab）。
   
   👉 另一個覺得很棒的思考方向：
   有沒有可能把 code 寫得簡明到不用註解？
   ⚠ 只辨解說沒寫註解就是好 code 👉 同事看了可能只能搖頭聳肩 ⚠
2. 命名。又來電腦科學最困難的問題，而且對台灣人來說，難的還有英文這個問題，難上又加難了，而且當初誰誰誰說要學程式的時候明明就沒講要練英文。
   
   📋 例如，想撈同桌裡最新的訂單，先想到 order = blah()，回頭再看時覺得可以幫讀者強調一下變數名稱，new_order？好一點點，但 latest_order 就更明白了，特別是當同桌可能同時有好張新訂單時。
   ● 這個例子剛好也聊到了對於「情境」與「文意」的認知，就從中文來看用字邏輯：「新」本來就不同義於「最新」。
3. 善用 _privatemethod、decorator（如最常見的 @classmethod 與 @staticmethod）告訴讀者說現在只需要管 input 與 output，大腦當下可以不用理會 class instance 的各種狀態值對這個函式的影響。
   
   ⭐️ 同時 classmethod 與 staticmethod 也都較一般的 instance method 更靜態。

靜態比動態容易預測

這次公司讀書會主題書《軟體架構工程｜工程方法》的第三章【模組化】有認真在說耦合性與共生性，也有討論到重構（refactor）的方向：建議由動態往靜態的方向重構（註 5）。

Python 後來版本出現的 Type Hint 與 pattern match，都帶讓程式更靜態、更容易預期的特徵。寫、讀程式時也可以試著去感受這個，從程式碼到系統架構都很重要的特徵。

由奢入簡難

要讓系統、程式變簡單是比較困難的事，所以嗅到把系統、程式變複雜的這種氣味時可以特別留意。

例如把東西通通放到 global 隨意用容易，而想把 global 的變數放到只有某些模組用的方向，就相對困難而需要花心思了。

Python 之禪又來了

"Namespaces are one honking great idea - let's do more of those!"

要說乾淨的 namespace 比混雜的好，原來 Senior 工程師的程式就是這麼樸實無華且枯燥 🍁。炫技錯了嗎？這可能問同事比較準。

常見的切檔名、namespace 有兩種主要的邏輯，一是由業務功能來切，一是由技術功能來切，這個在用各種 framework 時應該都會看到。

📋 例如 Django 第一層是切 app（業務功能），第二層是切 models、views 與 templates 等（技術功能）。

📋 而例如像把 API 切成兩個不要混成一個，這類基本招可能就看似枯燥，實則實用又優美，看出來的時候才能體會了。

關於動態與複雜的案例

邏輯相依於 Class 名稱還是 Enum 才好？也許是個很簡單的問題，平常沒事應該不會讓程式碼相依於 class 名稱？

💡 這恰恰也是動與靜的一個蠻好的思考點。一般使用了 enum 就是預期它的內容會被依賴，也因此在改動它時會有預期會有相關牽動，而在寫 Python 時，class 的名稱若非特別情境（例如 Django ORM 的 Model class 名稱會關聯到 DB 的 table 命名），可能會覺得說要換個 class 名稱的話，不就整個 code base 裡自己重構乾淨就好，比較不預期怎麼上線後會有 Celery queue 裡的 task 參數拿到上一版的改名前的值會噴錯。

💡 進一步討論可以再回頭思考 🐍 Python 的 🦆 duck typing，蠻推《Fluent Python, 2nd》裡的【Interfaces, Protocols, and ABCs】章節（連結附於文末）。

Dog.breath & Cat.breath，假如繼承的是 Breathable 然後每個繼承它的只是 breath 時會消耗不同的 air，這簡單的情境就沒有必要請 Dog 與 Cat 都重新實作 breath 並 super().breath()，可以請 Dog 與 Cat 各自定義自己的 air_consumed_per_breath 變數。

說起來也算是個簡單常見的物件導向程式概念，而 code review 時其實可以感受到「子類別需要自己重新實作 breath」的不確定感：它開啟了所有的可能 👉 程式變動態的味道增加了。

💣 例如沒想到子類別就自己寫了：

def breath(self):
    bark()

它不但沒有 super().breath() 還自己亂叫了，好不驚喜！
ㄟ…不要笑喔，估計是一般程式生涯 100% 會寫或看過的 code 少調用了 super 吧，儘請期待。

總結：累積好的 PR 造就好的 KR（key result）

不會讓三個月半年後的你自己 debug 到爆、使用者氣炸、營運團隊焦頭爛額的好程式，也同時讓每一個人的工作與生活都更簡單而輕鬆容易了些。善用一些 PR 原則與小技巧，讓大家都將更能專注在想完成的目標成果（例如職級期待） 🎯。

看到 PR 發出後，CI 在不預期的地方噴錯了

PR 剛完成時 CI 上幾個測試錯掉了，可能是相關規格沒有十分掌握的氣味（常見是規格或程式行為比預期的複雜）。因為專案不只是一個人的事，可以提醒 PR 作者開 ticket 提醒自己及／或請品質工程師 QE 同事驗證或上線前後一起注意一下👷。

愈 Senior 的工程師，留下的債愈少

也意味著未爆彈 💣 越少。而在 iCHEF 使用者夠多、使用時間夠久的情況下，🐛 bug 今年沒出來明年也會出來。

有人可能會問說怎麼實際上看到愈 Senior 的留的債愈多？先簡單回答：
一．他可能就是走過那一段路才成長過來的，畢竟誰沒有年輕過？（笑）
二．依各組織不同的工程人事文化……嗯，祝好運；可詳見彩蛋。

最後，也祝你看得一眼好 code、說得一嘴好 code 與寫得一手好 code。

⭐️ 小技巧：為了專案進度與團隊合作氣氛，睜一隻眼的時候，記得還有另一隻眼，可以用來閉上。當然也是說問題沒有大到不該放手的時候。

說好的🌈 彩蛋 🥚，與你的工作機會有關！iCHEF RD 在徵 iOS、後端職缺，連以前沒出現過的 Data Analyst／Engineer 職缺也將在 2023 年出現，也可以直接來參加 Taipei.py 台北 Python 使用者聚會洽詢主辦人（按 RSVP 投履歷？），疫情後我們又不小心開始每月認真辦活動囉，沒有想找工作也一樣歡迎來玩！

相關連結與參考閱讀：

1. 筆者之前一篇也蠻相關的主題：【Tech Design Review：軟體架構設計審查】進一步談了專案開發裡的架構設計。也推薦參考同 iCHEF 工程 blog 裡的【軟體工程師的架構設計】系列。
2. 筆者於 PyCon Taiwan 2021 研討會裡分享的講題：【成功地測試失敗】也談了使用測試輔助專案開發的心得。
3. Python 的進階教科書：《流暢的 Python：清晰、簡潔、有效的程式設計》 by Luciano Ramalho，正體中文還沒有第二版，幫 quote 一下第二版的新玩意兒：
   “type hints, data classes, and pattern matching — made this second edition almost 30% larger than the first.”
   — Fluent Python, 2nd 也順推 O’Reilly Learning 服務。
   ● Ch. 5. Data Class Builders（vs namedtuple、NamedTuple）
   ● Ch. 13. Interfaces, Protocols, and ABCs
   ● Ch. 15. More About Type Hints
4. 《Kent Beck的實作模式（Implementation Patterns）》
   ● 還記得 Python 的 unittest library 嗎？（假如竟然沒有每天在用的話……你用的是 pytest？……很棒！）它的老祖宗 JUnit 作者之一就是 Kent Beck，他跟 Erich Gamma 在長途飛機上沒事寫的。
5. 《軟體架構原理｜工程方法》
   Fundamentals of Software Architecture by Mark Richards, Neal Ford
   ● Ch. 3 Modularity 模組化
   ● Ch. 6 Measuring and Governing Architecture Characteristics 測量及管理架構特性，提到 Cyclomatic Complexity（CC）循環複雜度。
6. 關於《重構》的主題很重要，也不只有一大本書專門討論它，這邊就不詳述了。

很感謝同事 Johnny Lo 與 Sandi Wu 早期就先幫看草稿的回饋，旅美工程師 Kelly Chang 也給了許多不同角度的回饋，這篇文章還讀得下去的話，都是他們給了很大的幫助。