¿Tener una declaración de retorno solo para satisfacer la sintaxis es una mala práctica?

Considere el siguiente código:

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;
}

El return null;es necesario ya que se puede detectar una excepción, sin embargo, en tal caso, ya que ya verificamos si era nulo (y supongamos que sabemos que la clase a la que estamos llamando admite la clonación), por lo que sabemos que la declaración try nunca fallará.

¿Es una mala práctica poner la declaración de retorno adicional al final solo para satisfacer la sintaxis y evitar errores de compilación (con un comentario que explique que no se alcanzará), o hay una mejor manera de codificar algo como esto para que el extra ¿La declaración de devolución es innecesaria?

Una forma más clara sin una declaración de devolución adicional es la siguiente. Yo tampoco lo captaría CloneNotSupportedException, pero se lo dejé ir a la persona que llama.

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

Casi siempre es posible jugar con el orden para terminar con una sintaxis más sencilla que la que tiene inicialmente.

Definitivamente se puede alcanzar. Tenga en cuenta que solo está imprimiendo el seguimiento de pila en la catchcláusula.

En el escenario donde a != nully habrá una excepción, return null se alcanzará. Puede eliminar esa declaración y reemplazarla con throw new TotallyFooException();.

En general ^* , si nulles un resultado válido de un método (es decir, el usuario lo espera y significa algo), devolverlo como una señal de "datos no encontrados" o de que ocurrió una excepción no es una buena idea. De lo contrario, no veo ningún problema por el que no debería regresar null.

Tomemos, por ejemplo, el Scanner#ioExceptionmétodo:

Devuelve el IOExceptionúltimo arrojado por el legible subyacente de este escáner. Este método devuelve un valor nulo si no existe tal excepción .

En este caso, el valor devuelto nulltiene un significado claro, cuando uso el método puedo estar seguro de que lo obtuve nullsolo porque no hubo tal excepción y no porque el método intentó hacer algo y falló.

^* Tenga en cuenta que a veces desea volver nullincluso cuando el significado es ambiguo. Por ejemplo HashMap#get:

Un valor de retorno nulo no indica necesariamente que el mapa no contenga ningún mapeo para la clave; también es posible que el mapa asigne explícitamente la clave a nulo . La containsKeyoperación puede usarse para distinguir estos dos casos.

En este caso, nullpuede indicar que el valor null se encontró y se devolvió, o que el mapa hash no contiene la clave solicitada.

¿Es una mala práctica poner la declaración de retorno adicional al final solo para satisfacer la sintaxis y evitar errores de compilación (con un comentario que explique que no se alcanzará)?

Creo que return nulles una mala práctica para el término de una rama inalcanzable. Es mejor lanzar un RuntimeException( AssertionErrortambién sería aceptable) ya que para llegar a esa línea algo salió muy mal y la aplicación se encuentra en un estado desconocido. La mayoría de esto es (como arriba) porque el desarrollador se ha perdido algo (los objetos pueden ser no nulos y no clonables).

Es probable que no lo use a InternalErrormenos que esté muy seguro de que el código es inalcanzable (por ejemplo, después de a System.exit()), ya que es más probable que cometa un error que la VM.

Solo usaría una excepción personalizada (como TotallyFooException) si llegar a esa "línea inalcanzable" significa lo mismo que en cualquier otro lugar donde arroje esa excepción.

Atrapó lo CloneNotSupportedExceptionque significa que su código puede manejarlo. Pero después de detectarlo, literalmente no tiene idea de qué hacer cuando llega al final de la función, lo que implica que no podría manejarlo. Así que tienes razón en que es un olor a código en este caso, y en mi opinión significa que no debiste haberlo detectado CloneNotSupportedException.

Preferiría usar Objects.requireNonNull()para verificar si el parámetro a no es nulo. Entonces, cuando lee el código, queda claro que el parámetro no debe ser nulo.

Y para evitar las excepciones marcadas, volvería a lanzar el archivo CloneNotSupportedExceptioncomo RuntimeException.

Para ambos, puede agregar un texto agradable con la intención de por qué esto no debería suceder o ser el caso.

public Object getClone(Object a) {

    Objects.requireNonNull(a);

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

}

En este tipo de situación, escribiría

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) ; } }
}

Curiosamente, dice que la "declaración de prueba nunca fallará", pero aún así se tomó la molestia de escribir una declaración e.printStackTrace();que afirma que nunca se ejecutará. ¿Por qué?

Quizás su creencia no esté tan firmemente arraigada. Eso es bueno (en mi opinión), ya que su creencia no se basa en el código que escribió, sino en la expectativa de que su cliente no violará la condición previa. Es mejor programar los métodos públicos a la defensiva.

Por cierto, su código no se compilará para mí. No puedes llamar a.clone()incluso si el tipo de aes Cloneable. Al menos el compilador de Eclipse lo dice. La expresión a.clone()da error

El método clone () no está definido para el tipo Cloneable

Lo que haría por tu caso específico es

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

Donde PubliclyCloneablese define por

interface PubliclyCloneable {
    public Object clone() ;
}

O, si absolutamente necesita que el tipo de parámetro sea Cloneable, al menos se compila lo siguiente.

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) ; } }
}

Los ejemplos anteriores son válidos y muy Java. Sin embargo, así es como abordaría la pregunta del OP sobre cómo manejar esa devolución:

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

No hay ningún beneficio por verificar asi es nulo. Va a NPE. La impresión de un seguimiento de pila tampoco es útil. El seguimiento de la pila es el mismo independientemente de dónde se maneje.

No hay ningún beneficio en desechar el código con pruebas nulas inútiles y manejo de excepciones inútil. Al eliminar la basura, el problema de la devolución es discutible.

(Tenga en cuenta que el OP incluyó un error en el manejo de excepciones; es por eso que se necesitaba la devolución. El OP no se habría equivocado con el método que propongo).

¿Tener una declaración de retorno solo para satisfacer la sintaxis es una mala práctica?

Como han mencionado otros, en su caso esto no se aplica realmente.

Sin embargo, para responder a la pregunta, ¡los programas de tipo Lint seguramente no lo han descubierto! He visto a dos diferentes pelear por esto en una declaración de cambio.

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

Uno se quejó de que no tener la pausa era una violación del estándar de codificación. El otro se quejó de que tenerlo era uno porque era un código inalcanzable.

Me di cuenta de esto porque dos programadores diferentes seguían revisando el código con la interrupción agregada, luego eliminada, luego agregada y eliminada, dependiendo del analizador de código que ejecutaron ese día.

Si terminas en esta situación, elige uno y comenta la anomalía, que es la buena forma que mostraste. Esa es la mejor y más importante comida para llevar.

No es 'solo para satisfacer la sintaxis'. Es un requisito semántico del lenguaje que cada ruta de código conduzca a un retorno o un lanzamiento. Este código no cumple. Si se detecta la excepción, se requiere la siguiente devolución.

No hay "mala práctica" al respecto, o sobre la satisfacción del compilador en general.

En cualquier caso, ya sea sintáctico o semántico, no tiene otra opción.

Volvería a escribir esto para tener la devolución al final. Pseudocódigo:

if a == null throw ...
// else not needed, if this is reached, a is not null
Object b
try {
  b = a.clone
}
catch ...

return b

Nadie mencionó esto todavía, así que aquí va:

public static final Object ERROR_OBJECT = ...

//...

public Object getClone(Cloneable a) throws TotallyFooException {
Object ret;

if (a == null) 
    throw new TotallyFooException();

//no need for else here
try {
    ret = a.clone();
} catch (CloneNotSupportedException e) {
    e.printStackTrace();
    //something went wrong! ERROR_OBJECT could also be null
    ret = ERROR_OBJECT; 
}

return ret;

}

No me gustan los bloques returninteriores trypor esa misma razón.

El retorno nulo; es necesario ya que se puede capturar una excepción, sin embargo, en tal caso, dado que ya verificamos si era nulo (y supongamos que sabemos que la clase a la que llamamos admite la clonación), por lo que sabemos que la declaración try nunca fallará.

Si conoce los detalles sobre las entradas involucradas de una manera en la que sabe que la trydeclaración nunca puede fallar, ¿cuál es el punto de tenerla? Evite el trysi está seguro de que las cosas siempre van a tener éxito (aunque es raro que pueda estar absolutamente seguro durante toda la vida útil de su base de código).

En cualquier caso, el compilador desafortunadamente no es un lector de mentes. Ve la función y sus entradas, y dada la información que tiene, necesita esa returndeclaración en la parte inferior tal como la tiene.

¿Es una mala práctica poner la declaración de retorno adicional al final solo para satisfacer la sintaxis y evitar errores de compilación (con un comentario que explique que no se alcanzará), o hay una mejor manera de codificar algo como esto para que el extra ¿La declaración de devolución es innecesaria?

Todo lo contrario, sugeriría que es una buena práctica evitar las advertencias del compilador, por ejemplo, incluso si eso cuesta otra línea de código. No se preocupe demasiado por el recuento de líneas aquí. Establezca la confiabilidad de la función a través de pruebas y luego continúe. Simplemente fingiendo que podría omitir la returndeclaración, imagine volver a ese código un año después y luego tratar de decidir si esa returndeclaración en la parte inferior va a causar más confusión que un comentario que detalla los detalles de por qué se omitió debido a suposiciones que puede hacer sobre los parámetros de entrada. Lo más probable es que la returndeclaración sea más fácil de manejar.

Dicho esto, específicamente sobre esta parte:

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

Creo que hay algo un poco extraño con la mentalidad de manejo de excepciones aquí. Por lo general, desea aceptar las excepciones en un sitio donde tiene algo significativo que puede hacer en respuesta.

Puede pensar try/catchen un mecanismo de transacción. tryAl realizar estos cambios, si fallan y nos ramificamos en el catchbloque, haga esto (lo que sea que esté en el catchbloque) en respuesta como parte del proceso de reversión y recuperación.

En este caso, simplemente imprimir un seguimiento de pila y luego verse obligado a devolver un valor nulo no es exactamente una mentalidad de transacción / recuperación. El código transfiere la responsabilidad del manejo de errores a todas las llamadas de código getClonepara verificar manualmente las fallas. Quizás prefieras atrapar elCloneNotSupportedException y traducirlo a otra forma de excepción más significativa y lanzarla, pero no desea simplemente tragarse la excepción y devolver un nulo en este caso, ya que no es como un sitio de recuperación de transacciones.

Terminará por filtrar las responsabilidades a las personas que llaman para verificar manualmente y tratar la falla de esa manera, cuando lanzar una excepción evitaría esto.

Es como si cargaras un archivo, esa es la transacción de alto nivel. Puede que tengas un try/catchahí. Durante el proceso de tryingcarga de un archivo, puede clonar objetos. Si hay una falla en cualquier parte de esta operación de alto nivel (cargando el archivo), normalmente querrá lanzar excepciones hasta este try/catchbloque de transacciones de nivel superior para que pueda recuperarse con gracia de una falla al cargar un archivo (ya sea debido a un error en la clonación o cualquier otra cosa). Por lo tanto, generalmente no queremos tragarnos una excepción en un lugar granular como este y luego devolver un valor nulo, por ejemplo, ya que eso anularía gran parte del valor y el propósito de las excepciones. En su lugar, queremos propagar las excepciones hasta un sitio donde podamos tratarlas de manera significativa.

Su ejemplo no es ideal para ilustrar su pregunta como se indica en el último párrafo:

¿Es una mala práctica poner la declaración de retorno adicional al final solo para satisfacer la sintaxis y evitar errores de compilación (con un comentario que explique que no se alcanzará), o hay una mejor manera de codificar algo como esto para que el extra ¿La declaración de devolución es innecesaria?

Un mejor ejemplo sería la implementación del propio clon:

 public class A implements Cloneable {
      public Object clone() {
           try {
               return super.clone() ;
           } catch (CloneNotSupportedException e) {
               throw new InternalError(e) ; // vm bug.
           }
      }
 }

Aquí nunca se debe ingresar la cláusula catch . Aún así, la sintaxis requiere arrojar algo o devolver un valor. Dado que devolver algo no tiene sentido, InternalErrorse utiliza para indicar una condición de VM grave.