@levinzhang 2020-01-27T23:30:21.000000Z 字数 2894 阅读 572

我从上千次Code Review中学到了些什么

摘要

Code Review是保证代码质量的重要手段，本文作者Steven Heidel在LinkedIn积累了丰富的相关经验，在本文中他总结了常见的代码问题和修改方案。

本文最初发表于Hacker Noon站点，经原作者Steven Heidel同意由InfoQ中文站翻译分享。

当我在LinkedIn工作时，很大一部分工作内容都是进行code review。在这个过程中，我发现有些建议是反复出现的，因此我决定将它们总结到一个列表中并分享给团队。

如下是我所给出的三个（外加一个额外的福利）最常见的code review建议。

建议1：在出现问题时抛出异常

我经常看到这样的模式：

List<String> getSearchResults(...) {
  try {
    List<String> results = // 发起对搜索服务的REST调用
    return results;
  } catch (RemoteInvocationException e) {
    return Collections.emptyList();
  }
}

在我曾经参与的一个移动应用中，这种模式实际上会导致问题的出现。我们所使用的搜索后端已经开始抛出异常了，但是应用的API服务器中存在和上述示例类似的代码。因此，从应用的角度来看，它得到了成功的200响应，并且会对每个搜索查询都展现空列表。

如果API抛出异常的话，那么我们的监控系统会立刻发现它，并且能够及时得到处理。

很多时候，在捕获到异常之后，我们倾向于返回空对象。Java中空对象的样例包括Optional.empty()、null和空的列表。这种情况经常出现在URL解析中。如果URL无法从字符串解析得到的话，不要返回null，而是要问自己:

URL格式为何是不合法的？这是一个需要在上游解决的数据问题吗？

对于这种任务来说，空对象并不是恰当的工具。如果出现异常行为的话，那么就应该抛出异常。

建议二：尽可能使用更具体的类型

基本上来讲，这个建议恰好与字符串类型化（stringly typed programming）编程相反。

我经常看到如下面样例所示的代码：

void doOperation(String opType, Data data); 
// 在这里，opType为“insert”、“append”或“delete”，如果是枚举的话，会更加清晰
String fetchWebsite(String url);
// 在这里，url是“https://google.com”，它应该是URN类型
String parseId(Input input);
// 返回类型是String，但是id实际上是像“6345789”这样的Long类型

使用最具体的类型可以避免很多的bug，这基本上就是选择Java这样的强类型语言的全部原因。

所以现在的问题是：好心的程序员为什么会写出糟糕的字符串类型的代码呢？答案在于外部世界不是强类型的。字符串有很多不同的来源，比如：

url中的查询和路径参数
JSON
不支持枚举的数据库
编写糟糕的库

在上述的所有场景中，我们应该使用如下的策略来避免该问题：将字符串解析和序列化放在程序的边缘之处。

如下是一个样例：

// 第一步：获得代表公司名/会员id的查询参数并对其进行解析
// 示例：context=Pair(linkedin,456)
Pair<String, Long> companyMember = parseQueryParam("context");
// 如果格式不正确的话，应该抛出异常
// 第二步：完成我们的应用的所有任务
// 即便不是所有，但大多数我们的代码要位于此处
// 第三步：如果需要的话，在最后将参数转换回String
String redirectLink = serializeQueryParam("context");

这种方式会带来许多好处。立即发现不符合格式的数据；如果出现任何问题的话，应用程序将提前失败。数据被验证一次之后，我们不必在整个应用程序中捕获解析异常。此外，强类型使方法签名更具描述性，我们不再需要在每个方法上编写那么多的javadoc。

建议三：使用Optional来替代null

Java 8所带来的最棒的特性之一就是Optional类，它代表一个可能存在也可能不存在的实体。

小问题：唯一具备缩写形式的异常是哪一个？答案是NPE或者说是空指针异常。到目前为止，它是Java中最常见的异常，被称为价值10亿美元的错误。

Optional能够让我们完全从程序中移除NPE。但是，必须以正确的方式使用它。如下是关于使用Optional的一些建议：

我们不能在得到Optional的任何时候都简单地调用它的.get()，相反，我们要仔细考虑Optional不存在的情况并给出一个合理的默认值；
如果还没有合理的默认值，那么像.map()和.flatmap()这样的方法允许我们推迟到以后再做决定；
如果外部库返回null来表示为空的情况，那么立即使用Optional.ofNullable()包装该值。相信我，你以后会感谢自己的。null值在程序内部有“冒泡”的倾向，所以最好在源代码中停止它们；
在方法的返回类型中使用Optional。这种做法非常好，因为我们不需要读取javadoc来确定值是否可能不存在。

额外的建议：尽可能对方法扁平化（unlift）

我们应该避免如下所示的方法：

// 避免这样做：
CompletableFuture<T> method(CompletableFuture<S> param);
// 推荐这样做：
T method(S param);
// 避免这样做：
List<T> method(List<S> param);
// 推荐这样做：
T method(S param);
// 避免这样做：
T method(A param1, B param2, Optional<C> param3);
// 推荐这样做：
T method(A param1, B param2, C param3);
T method(A param1, B param2);
// 显然，该方法做了两件事，它应该拆分为两个方法
// 对于boolean参数同样如此

这些不推荐使用的方法有什么共同点呢？那就是它们都使用了容器对象作为参数，如Optional、List或Task。如果返回类型是相同种类的容器，那就更糟糕了（比如，方法参数接收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”恰好相反。采用这种方式进行重写会让方法更具灵活性，对调用者更加易用。

其他参考材料：
实用的函数式编程