做了 1000 次 Code Review，我学到 3 点经验

代码审查| 2020-06-01

图0：做了 1000 次 Code Review，我学到 3 点经验

当我在 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 亿美元的错误。

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”恰好相反。采用这种方式进行重写会让方法更具灵活性，对调用者更加易用。

本文文字及图片出自 InfoQ