仅为满足语法而添加返回语句是一种不良实践吗？

Question

请考虑以下代码：

public Object getClone(Cloneable a) throws TotallyFooException {

    if (a == null) {
        throw new TotallyFooException();
    }
    else {
        try {
            return a.clone();
        } catch (CloneNotSupportedException e) {
            e.printStackTrace();
        }
    }
    //cant be reached, in for syntax
    return null;
}

return null; 是必要的，因为可能会捕获异常，但在这种情况下，由于我们已经检查过它是否为null，并假设我们知道调用的类支持克隆，因此我们知道try语句永远不会失败。

在最后加上额外的返回语句并附带一条注释来解释其不会被执行，是不是一种不好的做法以满足语法并避免编译错误？还是有更好的编码方式可以避免多余的返回语句？

- yitzih

---

你的方法接受一个“Object”参数。如果“A”不是支持“clone”方法的类（或者这个方法是在“Object”中定义的？），或者在执行“clone”方法时发生错误（或者发生其他我现在想不到的任何错误），将会抛出异常并达到返回语句。 - Arc676

你认为无法达到最终返回是错误的假设。对于实现了“Cloneable”接口的类抛出“CloneNotSupportedException”异常是完全合法的。来源：javadocs - Cephalopod

你所评论为“无法到达”的代码是可以到达的。如上所述，从CloneNotSupportedException到达此行存在代码路径。 - David Waterworth

核心问题在于：如果您假设所调用的类支持克隆，为什么还要捕获异常？异常应该只在出现异常行为时才会被抛出。 - deworde

@wero 我会选择使用 AssertionError 而不是 InternalError。后者似乎是用于特殊情况，如果 JVM 中出现问题。而你基本上断言代码不会被执行到。 - kap

Answer 1

一个更清晰的方式，不需要额外的返回语句，如下所示。我也不会捕获 CloneNotSupportedException，但会将其传递给调用者。

if (a != null) {
    try {
        return a.clone();
    } catch (CloneNotSupportedException e) {
        e.printStackTrace();
    }
}
throw new TotallyFooException();

几乎总是可以通过调整顺序，得到比最初更加简单直观的语法。

- Kayaman

---

这说明了一个非常重要的观点。如果你发现自己在与Java的要求作斗争，通常意味着你正在尝试以次优的方式去做某些事情。通常，当我感到不舒服时，我会退后一步，审视整个局面，并尝试找出是否可以用不同的方式编写代码，使其更清晰易懂 - 通常是有可能的。一旦你接受了Java的大部分细枝末节，它们都会带来惊人的帮助。 - Bill K

@BillK：正如 Maroun Maroun 指出的那样，这段代码有不同的语义。 - Deduplicator

AssertionError 是一个内置类，其意图类似于 TotallyFooException（实际上并不存在）。 - user253751

如果你发现自己在与Java的要求进行斗争，通常意味着你正在试图用一种次优的方式来做某件事情。或者是Java决定不支持某个可以让你生活更轻松的功能。 - user541686

@Mehrdad，...但是以那种方式看待它在实际应用中没有什么实质性的帮助，除非你是一个为未来Java版本或替代语言工作的人。学习当地的习语有真正实用的优势，而责备语言（任何语言，除非它具有元编程支持，比如LISP允许自己添加新的语法）不支持你最初想要完成某事的方式是没有意义的。 - Charles Duffy

Answer 2

它绝对可以达到。请注意，在 catch 子句中仅打印堆栈跟踪。

在发生异常的情况下，a != null 并且将会有一个异常，return null 将会被执行。您可以删除该语句，并将其替换为 throw new TotallyFooException();。

通常来说^*，如果 null 是方法的 有效 结果（即用户 期望 它并且它具有一定的含义），那么将其作为“未找到数据”或出现异常的信号返回是不明智的。否则，我认为你不应该返回 null 会有任何问题。

以 Scanner#ioException 方法为例:

  

返回此 Scanner 的底层 Readable 最后抛出的 IOException。如果不存在这样的异常，则此方法返回null。

在这种情况下，返回值 null 具有明确的含义，当我使用该方法时，我可以确定之所以得到 null 是因为没有这样的异常，而不是因为方法尝试做某事并失败了。

^*请注意，有时即使含义不明确，您也希望返回 null。例如 HashMap#get:

返回值为null并不一定表示该映射中不包含键对应的映射，也有可能是该映射将该键显式映射为null。可以使用containsKey操作来区分这两种情况。

在这种情况下，null可能表示找到并返回了null值，也可能表示哈希映射不包含所请求的键。

- Maroun

---

虽然你的观点是有道理的，但我想指出这只是糟糕的 API 设计。这两种方法应分别返回 Optional<Throwable> 或 Optional<TValue>。注意：这并不是对你答案的批评，而是对这两个方法的 API 设计的批评。（然而，这是一种相当普遍的设计错误，在不仅仅是整个 Java API 中广泛存在，还包括 .NET 等其他语言的 API 设计中）。 - Jörg W Mittag

如果你不确定能否使用Java 8的功能，那么"crappy"这个词有点过于严厉了。 - Thorbjørn Ravn Andersen

但是当null不是一个有效的返回值时，返回null甚至更糟。换句话说，在意外情况下返回null永远不是一个好主意。 - Holger

@Holger 我所说的“无效”是指例如当用户期望从一个不能包含null的数据结构中获取值时，null永远不可能是“有效”的返回值，因为它必须表示其他东西而不是正常的返回值。在这种情况下，返回它具有特殊的含义，我认为这种设计没有任何问题（当然用户应该知道如何处理这种情况）。 - Maroun

这就是关键：“用户应该知道如何处理这种情况”意味着你必须指定 null 是一个可能的返回值，调用者必须处理。这使得它成为一个有效的值，我们对“有效”的理解与你描述的“有效”相同，“用户期望它并且它有意义”。但在这种情况下，情况已经不同了。OP根据代码注释声明他断言 return null; 语句“无法到达”，这意味着返回 null 是错误的。而应该使用 throw new AssertionError();。 - Holger

Answer 3

“在结尾添加额外的返回语句只是为了满足语法要求并避免编译错误（附带解释说明它不会被执行），这样做是否是不好的实践？” 我认为在无法到达的分支终点使用“return null”是不好的实践。更好的做法是抛出RuntimeException（也可以接受AssertionError），因为到达该行意味着应用程序处于未知状态，出现了严重问题。通常情况下，这是因为开发人员忽略了某些内容（对象可能为非null且不可克隆）。 除非我非常确定代码是无法到达的（例如在System.exit()之后），否则我可能不会使用InternalError，因为我犯错的可能性比虚拟机更大。

如果到达"无法到达的代码行"意味着和其他抛出该异常的地方一样，那么我只会使用自定义异常（例如TotallyFooException）。

- Michael Lloyd Lee mlk

---

在评论中的其他地方已经提到，AssertionError也是在发生"can't happen"事件时抛出的一个合理选择。 - Ilmari Karonen

个人而言，我会抛出一个 SomeJerkImplementedClonableAndStillThrewACloneNotSupportedExceptionException 异常。显然，它会继承 RuntimeException。 - w25r

@w25r 我没有使用上面的代码，但是回答了潜在的问题。由于 Cloneable 没有实现公共的 clone() 方法，并且对象中的 clone() 是 protected 的，上面的代码实际上无法编译。 - Michael Lloyd Lee mlk

我不会使用被动攻击性异常，以防它逃脱（尽管为我们的客户之一获得“JerkException”可能很有趣，但我认为这不会受到赞赏）。 - Michael Lloyd Lee mlk

Answer 4

你捕获了CloneNotSupportedException，这意味着你的代码可以处理它。但是，在你捕获它之后，当你到达函数末尾时，你真的不知道该做什么，这意味着你无法处理它。因此，在这种情况下，你是正确的，这是一种代码异味，并且在我的看法中意味着你不应该捕获CloneNotSupportedException。

- djechlin

Answer 5

我更喜欢使用Objects.requireNonNull()来检查参数a是否为null。这样在阅读代码时清楚地表明参数不应为null。

为了避免受检异常，我会将CloneNotSupportedException重新抛出为RuntimeException。

对于两者，您可以添加说明文字，解释为什么不应该发生或成为案例。

public Object getClone(Object a) {

    Objects.requireNonNull(a);

    try {
        return a.clone();
    } catch (CloneNotSupportedException e) {
        throw new IllegalArgumentException(e);
    }

}

- Kuchi

Answer 6

上面的示例是有效的并且非常符合Java语言规范。然而，以下是我如何解决原帖中关于如何处理返回值的问题：

public Object getClone(Cloneable a) throws CloneNotSupportedException {
    return a.clone();
}

检查 a 是否为空并没有任何好处。这会导致 NPE。打印堆栈跟踪也没有帮助。无论在哪里处理，堆栈跟踪都是相同的。

通过删除无用的空值检查和异常处理来简化代码，可以避免返回问题。

（请注意，OP 在异常处理中包含了一个错误，这就是为什么需要返回的原因。如果使用我提出的方法，OP 就不会犯错。）

- Tony Ennis

Answer 7

在这种情况下，我会写成：

public Object getClone(SomeInterface a) throws TotallyFooException {
    // Precondition: "a" should be null or should have a someMethod method that
    // does not throw a SomeException.
    if (a == null) {
        throw new TotallyFooException() ; }
    else {
        try {
            return a.someMethod(); }
        catch (SomeException e) {
            throw new IllegalArgumentException(e) ; } }
}

有趣的是，你说“try语句永远不会失败”，但你仍然费心编写了一条语句e.printStackTrace();，声称这条语句永远不会被执行。为什么？

也许你对此并没有坚定的信念。在我看来这是好事，因为你的信念并不是基于你编写的代码，而是基于你的客户不会违反前提条件的期望。最好为公共方法进行防御式编程。

顺便说一下，你的代码在我这里无法编译。即使a的类型为Cloneable，你也不能调用a.clone()。至少Eclipse的编译器是这样说的。表达式a.clone()会出错：

The method clone() is undefined for the type Cloneable

针对你的特定情况，我会这样做：

public Object getClone(PubliclyCloneable a) throws TotallyFooException {
    if (a == null) {
        throw new TotallyFooException(); }
    else {
        return a.clone(); }
}

其中PubliclyCloneable的定义如下：

interface PubliclyCloneable {
    public Object clone() ;
}

或者，如果您绝对需要参数类型为Cloneable，则以下代码至少可以编译通过。

public static Object getClone(Cloneable a) throws TotallyFooException {
//  Precondition: "a" should be null or point to an object that can be cloned without
// throwing any checked exception.
    if (a == null) {
        throw new TotallyFooException(); }
    else {
        try {
            return a.getClass().getMethod("clone").invoke(a) ; }
        catch( IllegalAccessException e ) {
            throw new AssertionError(null, e) ; }
        catch( InvocationTargetException e ) {
            Throwable t = e.getTargetException() ;
            if( t instanceof Error ) {
                // Unchecked exceptions are bubbled
                throw (Error) t ; }
            else if( t instanceof RuntimeException ) {
                // Unchecked exceptions are bubbled
                throw (RuntimeException) t ; }
            else {
                // Checked exceptions indicate a precondition violation.
                throw new IllegalArgumentException(t) ; } }
        catch( NoSuchMethodException e ) {
            throw new AssertionError(null, e) ; } }
}

- Theodore Norvell

---

编译时错误让我想知道为什么Cloneable没有将clone声明为一个不抛出任何已检查异常的公共方法。在http://bugs.java.com/view_bug.do?bug_id=4098033上有一个有趣的讨论。 - Theodore Norvell

我想提一下，绝大多数Java代码不使用类似Lisp的大括号，如果你在工作环境中尝试使用它，你会被人瞪眼。 - anon

谢谢。实际上我并不是很一致。我编辑了答案，以便始终使用我喜欢的大括号风格。 - Theodore Norvell

Answer 8

仅仅为了满足语法需要而使用返回语句是不良实践吗？

正如其他人所提到的，在您的情况下，这并不适用。

然而，为了回答这个问题，Lint类型的程序肯定还没有搞清楚！我曾经看到两个不同的程序在switch语句中对此进行争论。

    switch (var)
   {
     case A:
       break;
     default:
       return;
       break;    // Unreachable code.  Coding standard violation?
   }

其中一位程序员抱怨缺少此处的间断会违反编码标准，而另一位则抱怨有此间断反而是不可到达代码。

我之所以注意到这点，是因为两个不同的程序员在根据他们当天运行的代码分析器来回添加和删除该间断。

如果你遇到这种情况，请选择一个并注释异常情况，这就是展示自己良好形式的最佳和最重要的收获。

- Jos

Answer 9

这不仅仅是为了满足语法要求，它是语言的一个语义要求，即每个代码路径都必须导致返回或抛出异常。这段代码不符合要求。如果捕获了异常，则需要跟随一个返回。

这与“满足编译器”没有任何关系，也不是什么“不良实践”。

无论是语法还是语义，你都没有选择。

- user207421

Answer 10

return null;语句是必要的，因为可能会捕获异常，但在这种情况下，由于我们已经检查过它是否为空（并且假设我们知道我们正在调用的类支持克隆），所以我们知道try语句永远不会失败。

如果您了解有关输入的详细信息，并且知道try语句永远不会失败，那么使用它有什么意义呢？如果您确定事情总是会成功，避免使用try（虽然很少能够完全确定代码库的整个生命周期）。

无论如何，编译器不幸地不能读取思维。它看到函数及其输入，并根据其拥有的信息需要底部的return语句。

相反，我建议避免任何编译器警告，例如，即使这会增加一行代码。在这里不要太担心行数。通过测试确立函数的可靠性，然后继续进行。假装您可以省略底部的return语句，想象一年后回到该代码，然后尝试决定底部的return语句是否会比某些详细说明为什么将其省略更容易引起混淆，因为您可以对输入参数进行假设。最有可能的是，return语句会更容易处理。

也就是说，特别是关于这部分：

try {
    return a.clone();
} catch (CloneNotSupportedException e) {
   e.printStackTrace();
}
...
//cant be reached, in for syntax
return null;

我认为这里的异常处理思维方式稍有不对。通常情况下，你希望在有意义的响应时吞掉异常。

你可以将try/catch视为事务机制。试图做出改变，如果失败并进入catch块，则作为回滚和恢复过程的一部分，在响应中执行catch块中的内容。

在这种情况下，仅仅打印堆栈跟踪然后被迫返回null并非是事务/恢复思维方式。代码将错误处理责任转移给所有调用getClone的代码来手动检查失败。你可能更喜欢捕获CloneNotSupportedException，然后将其转换为另一种更有意义的异常形式并抛出该异常，但在这种情况下，你不想简单地吞下异常并返回null，因为这不像是一个事务恢复网站。

如果这样做，你最终会将责任泄漏到调用方手动检查和处理失败，而抛出异常则可以避免这种情况。

这就像你加载一个文件，那就是高层次的事务。你可能在那里使用try/catch。在尝试加载文件的过程中，你可能会克隆对象。如果在这个高层操作（加载文件）的任何地方出现故障，通常希望将异常一路抛回到这个顶层事务try/catch块，以便你可以从加载文件失败中优雅地恢复（无论是由于克隆错误还是其他任何原因）。所以我们通常不希望在这样一个细粒度的地方吞下异常然后返回null，因为那将毁掉异常的很多价值和目的。相反，我们希望将异常传播回到一个有意义处理它的站点。

- user4842163