Categories
程式開發

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


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

当我在LinkedIn工作时,工作的很大一部分内容是做Code Review。在这个过程中,我发现一些人很容易犯的错误,于是把它们整理起来并分享给团队。

经验1:当出现错误时Throw an exception

我看到的一个常见模式是:

List getSearchResults(...) {
  try {
    List 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 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 method(CompletableFuture param);
// PREFER: 
T method(S param);

// AVOID:
List method(List param);
// PREFER:
T method(S param);

// AVOID: 
T method(A param1, B param2, Optional 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 method(Promise param)要比2)A method(B param)更缺少灵活性。

如果有一个Promise的话,我们可以用1),也能通过.map函数使用2)(即promise.map(method))。

但是,如果只有一个B的话,我们很容易使用2),但是无法使用1),这样来看,2)是更具灵活性的方案。

我喜欢将其称为“unlifting”,因为它与常见的函数式工具方法“lift”恰好相反。采用这种方式进行重写会让方法更具灵活性,对调用者更加易用。

英文原文:

What I Learned From Doing 1000 Code Reviews