@levinzhang
2020-01-27T23:30:21.000000Z
字数 2894
阅读 561
Code Review是保证代码质量的重要手段,本文作者Steven Heidel在LinkedIn积累了丰富的相关经验,在本文中他总结了常见的代码问题和修改方案。
本文最初发表于Hacker Noon站点,经原作者Steven Heidel同意由InfoQ中文站翻译分享。
当我在LinkedIn工作时,很大一部分工作内容都是进行code review。在这个过程中,我发现有些建议是反复出现的,因此我决定将它们总结到一个列表中并分享给团队。
如下是我所给出的三个(外加一个额外的福利)最常见的code review建议。
我经常看到这样的模式:
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这样的强类型语言的全部原因。
所以现在的问题是:好心的程序员为什么会写出糟糕的字符串类型的代码呢?答案在于外部世界不是强类型的。字符串有很多不同的来源,比如:
在上述的所有场景中,我们应该使用如下的策略来避免该问题:将字符串解析和序列化放在程序的边缘之处。
如下是一个样例:
// 第一步:获得代表公司名/会员id的查询参数并对其进行解析
// 示例:context=Pair(linkedin,456)
Pair<String, Long> companyMember = parseQueryParam("context");
// 如果格式不正确的话,应该抛出异常
// 第二步:完成我们的应用的所有任务
// 即便不是所有,但大多数我们的代码要位于此处
// 第三步:如果需要的话,在最后将参数转换回String
String redirectLink = serializeQueryParam("context");
这种方式会带来许多好处。立即发现不符合格式的数据;如果出现任何问题的话,应用程序将提前失败。数据被验证一次之后,我们不必在整个应用程序中捕获解析异常。此外,强类型使方法签名更具描述性,我们不再需要在每个方法上编写那么多的javadoc。
Java 8所带来的最棒的特性之一就是Optional
类,它代表一个可能存在也可能不存在的实体。
小问题:唯一具备缩写形式的异常是哪一个?答案是NPE或者说是空指针异常。到目前为止,它是Java中最常见的异常,被称为价值10亿美元的错误。
Optional
能够让我们完全从程序中移除NPE。但是,必须以正确的方式使用它。如下是关于使用Optional
的一些建议:
Optional
的任何时候都简单地调用它的.get()
,相反,我们要仔细考虑Optional
不存在的情况并给出一个合理的默认值;.map()
和.flatmap()
这样的方法允许我们推迟到以后再做决定;null
来表示为空的情况,那么立即使用Optional.ofNullable()
包装该值。相信我,你以后会感谢自己的。null值在程序内部有“冒泡”的倾向,所以最好在源代码中停止它们;Optional
。这种做法非常好,因为我们不需要读取javadoc来确定值是否可能不存在。我们应该避免如下所示的方法:
// 避免这样做:
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”恰好相反。采用这种方式进行重写会让方法更具灵活性,对调用者更加易用。
其他参考材料:
实用的函数式编程