做了1000次Code Review,我學到這3點經驗
Code Review 是保證代碼質量的重要手段。Steven Heidel 曾在 LinkedIn 負責 Code Review,他在本文總結了常見的代碼問題并提出修改方案。
當我在 LinkedIn 工作時,工作的很大一部分內容是做 Code Review。在這個過程中,我發現一些人很容易犯的錯誤,于是把錯誤整理起來并分享給團隊。
經驗 1:當出現錯誤時 Throw an exception
我看到的一個常見模式是:
- List<String> getSearchResults(...) {
- try {
- List<String> results = // make REST call to search service
- return results;
- } catch (RemoteInvocationException e) {
- return Collections.emptyList();
- }
- }
上面的方法可能是很多新手工程師的做法,但這種模式會有問題。在我曾經參與的移動應用中,這種模式導致移動應用程序的故障。用戶搜索開始后,我們的后端發生錯誤開始 throwing exceptions,但在應用程序的 API server 中并沒有 throwing exceptions。
因此,從應用角度看,前端會收到 200 個成功的響應,然后顯示空白的搜索結果給使用者,而團隊卻毫不知情。
如果 API thrown an exception,那我們的監控系統會立刻發現它,并能及時修復。
很多時候,當捕捉到異常后,我們傾向于返回 empty object。Java 中 empty object 的樣例包括 Optional.empty()、null 和 empty list。這種情況經常發生在 URL 解析中。如果 URL 無法從字符串解析得到的話,不要返回 null,而要問問自己:
URL 格式為什么是不合法的?這是一個需要在 upstream 解決的數據問題嗎?
對于這種任務來說,empty object 并不是恰當的工具。如果出現異常行為,那么就應該 throw an exception。
經驗 2:盡可能使用最具體的類型(type)
基本而言,這條建議恰好與 stringly typed programming 相反。
我經常看到下面所示的代碼:
- void doOperation(String opType, Data data);
- // where opType is "insert", "append", or "delete", this should have clearly been an enum
- String fetchWebsite(String url);
- // where url is "https://google.com", this should have been an URN
- String parseId(Input input);
- // the return type is String but ids are actually Longs like "6345789"
用最具體的類型 (type)可以避免很多 bug。
現在問題是:好心的程序員為什么會寫出糟糕的 stringly typed 代碼?
答案在于外部世界不是強類型的。字符串有很多不同的來源,比如:
- url 中的查詢和路徑參數
- JSON
- 不支持枚舉的數據庫
- 編寫糟糕的庫
在上述場景中,我們應使用如下的策略來避免該問題:將字符串解析和序列化放在程序的邊緣之處。
下面是這樣一個樣例:
- // Step 1: Take a query param representing a company name / member id pair and parse it
- // example: context=Pair(linkedin,456)
- Pair<String, Long> companyMember = parseQueryParam("context");
- // this should throw an exception if malformed
- // Step 2: Do all the stuff in your application
- // MOST if not all of your code should live in this area
- // Step 3: Convert the parameter back into a String at the very end if necessary
- String redirectLink = serializeQueryParam("context");
這種方式有很多優點。立即發現格式錯誤的數據;如果出現任何問題,應用程序將提前 fails。數據被驗證一次后,不必在整個應用程序中繼續捕獲解析異常。
此外,強類型使方法簽名更具描述性,我們不再需要在每個方法上編寫那么多的 javadocs。
經驗 3:用 Optionals 而非 nulls
Java 8 帶來最棒的特性之一是Optional類,它代表一個可能存在也可能不存在的實體。
一個小問題:
唯一擁有自己縮寫的例外(exception)是什么?答案是 NPE 或空指針異常。截至目前,它是 Java 中最常見的異常,并被稱為價值 10 億美元的錯誤 (https://www.infoq.com/presentations/Null-References-The-Billion-Dollar-Mistake-Tony-Hoare )。
Optional能讓我們完全從程序中移除 NPE。但是,必須以正確的方式使用它。如下是關于使用Optional的一些建議:
- 我們不能在得到Optional的任何時候都簡單地調用它的.get(),相反,我們要仔細考慮Optional不存在的情況并給出一個合理的默認值;
- 如果還沒有合理的默認值,那么像.map()和.flatmap()這樣的方法允許我們推遲到以后再做決定;
- 如果外部庫返回null來表示為空的情況,那么立即使用Optional.ofNullable()wrap 該值。相信我,你以后會感謝自己的。null 值在程序內部有“bubble up”的傾向,所以最好在源代碼中停止它們;
- 在方法的返回類型中使用Optional。這種做法非常好,因為我們不需要讀取 javadoc 來確定值是否可能不存在。
額外建議:盡可能采用“Unlift”方法
我們應避免下面所示的方法:
- // AVOID:
- CompletableFuture<T> method(CompletableFuture<S> param);
- // PREFER:
- T method(S param);
- // AVOID:
- List<T> method(List<S> param);
- // PREFER:
- T method(S param);
- // AVOID:
- T method(A param1, B param2, Optional<C> param3);
- // PREFER:
- T method(A param1, B param2, C param3);
- T method(A param1, B param2);
- // This method is clearly doing two things, it should be two methods
- // The same is true for boolean parameters
上述不推薦使用的方法有哪些共同點?那就是它們都使用了 container objects 作為參數,比如 Optional、List 或 Task。
如果返回類型是相同種類的 container,那就更糟糕了(比如,param methods 接收 Optional,返回值也是 Optional)。
為什么呢?
1)Promise<A> method(Promise<B> param)要比 2)A method(B param)更缺少靈活性。
如果有一個Promise<B>的話,我們可以用 1),也能通過.map函數使用 2)(即promise.map(method))。
但是,如果只有一個 B 的話,我們很容易使用 2),但是無法使用 1),這樣來看,2) 是更具靈活性的方案。
我喜歡將其稱為“unlifting”,因為它與常見的函數式工具方法“lift”恰好相反。采用這種方式重寫會讓方法更具靈活性,對調用者更加易用。