¿Tener una sentencia return solo para satisfacer la sintaxis es una mala práctica?

Definitivamente se puede alcanzar. Tenga en cuenta que solo está imprimiendo el stacktrace en la cláusula catch.

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

En general^*, si null es a válido resultado de un método (es decir, el usuario espera y significa algo) y luego devolverlo como una señal para el "dato no encontrado" o la excepción ocurrida es no una buena idea. De lo contrario, no veo ningún problema por qué no debería volver null.

Tomemos por ejemplo el Scanner#ioException método:

Devuelve el IOException último lanzado por el subyacente legible de este Escáner. Este método devuelve null si tal excepción no existe.

En este caso, el valor devuelto null tiene un significado claro, cuando uso el método puedo estar seguro de que solo obtuve null porque no hubo tal excepción y no porque el método trató de hacer algo y fracasó.

^*Tenga en cuenta que a veces desea devolver null incluso cuando el significado es ambiguo. Por ejemplo, el HashMap#get:

Un valor devuelto de null no indica necesariamente que el mapa no contiene ninguna asignación para la clave; también es posible que el mapa mapee explícitamente la clave a null . La operación containsKey puede ser se utiliza para distinguir estos dos casos.

En este caso null puede indicar que el valor null fue encontrado y devuelto, o que el hashmap no contiene la clave solicitada.

Es una mala práctica poner la instrucción extra return al final solo para satisfacer la sintaxis y evitar errores de compilación (con un comentario explicando que no se alcanzará)

Creo que return null es una mala práctica para el término de una rama inalcanzable. Es mejor lanzar una RuntimeException (AssertionError también sería aceptable) como para llegar a la línea que algo ha ido muy mal y la aplicación está en un estado desconocido. La mayoría como esto es (como arriba) porque el desarrollador ha perdido algo (los objetos pueden ser none-null y un-cloneable).

Probablemente no usaría InternalError a menos que esté muy, muy seguro de que el código es inalcanzable (por ejemplo, después de un 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 lance esa excepción.

Atrapaste el CloneNotSupportedException lo que significa que tu código puede manejarlo. Pero después de atraparlo, literalmente no tienes idea de qué hacer cuando llegas al final de la función, lo que implica que no podrías 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 deberías haber atrapado CloneNotSupportedException.

Preferiría usar Objects.requireNonNull() para comprobar si el Parámetro a no es null. Así que está claro cuando lees el código que el parámetro no debe ser null.

Y para evitar Excepciones comprobadas volvería a lanzar el CloneNotSupportedException como un RuntimeException.

Para ambos podría 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 yo escribiría{[12]]}

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, usted dice que la "declaración try nunca fallará", pero aún así se tomó la molestia de escribir una declaración e.printStackTrace(); que afirma que nunca será ejecutada. ¿Por qué?

Tal vez su creencia no está firmemente sostenida. 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. Mejor programar métodos públicos defensivamente.

Por cierto, tu código no se compilará para mí. No puedes llamar a a.clone() incluso si el tipo de a es 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 su caso específico es{[12]]}

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

Donde PubliclyCloneable se define por

interface PubliclyCloneable {
    public Object clone() ;
}

O, si es absolutamente necesario que el tipo de parámetro sea Cloneable, al menos lo siguiente compilar.

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 de la OP sobre cómo manejar ese retorno:

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

No hay beneficio para comprobar a para ver si es null. Va a NPE. Imprimir 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 para juntar el código con pruebas null inútiles y manejo de excepciones inútiles. Al eliminar la basura, el problema de devolución es discutible.

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

¿Tener una sentencia return solo para satisfacer la sintaxis es una mala práctica?

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

Para responder a la pregunta, sin embargo, los programas de tipo Pelusa seguro que no lo han descubierto! He visto a dos diferentes pelear por esto en una declaración de switch.

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

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

Me di cuenta de esto porque dos programadores diferentes seguían revisando el código con el break agregado, luego eliminado, luego agregado y luego eliminado, dependiendo del analizador de código que ejecutaran ese día.

Si terminas en esta situación, elige una 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 conduce a un retorno o un lanzamiento. Este código no cumple. Si se detecta la excepción, se requiere una devolución siguiente.

No hay 'mala práctica' al respecto, o sobre satisfacer al compilador en general.

En cualquier caso, ya sea sintaxis o semántica, no tiene ninguna opción al respecto.

Reescribiría esto para tener el retorno 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 gusta return dentro de try bloques por esa misma razón.

El retorno null; es necesario ya que una excepción puede ser capturada, sin embargo, en tal caso, ya que comprobamos si era null (y supongamos que sabemos que la clase a la que estamos llamando admite la clonación) sepa que la declaración try nunca fallará.

Si conoce los detalles sobre las entradas involucradas de una manera en la que sabe que la declaración try nunca puede fallar, ¿cuál es el punto de tenerla? Evita el try si sabes con certeza que las cosas siempre van a tener éxito (aunque es raro que pueda estar absolutamente seguro durante toda la vida 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 instrucción return 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 no se llegará a explicar), o ¿hay una mejor manera de codificar ¿algo como esto para que la declaración de retorno adicional sea innecesaria?

Por el contrario, sugiero que es una buena práctica evitar cualquier advertencia del compilador, por ejemplo, incluso si eso cuesta otra línea de código. No te preocupes demasiado por el número 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 declaración return, imagine volver a ese código un año después y luego tratar de decidir si esa declaración return en la parte inferior va a causar más confusión que algún comentario que detalla la minucia de por qué se omitió debido a las suposiciones que puede hacer sobre los parámetros de entrada. Lo más probable es que la declaración return sea más fácil de tratar.

Dicho esto, específicamente acerca de esta parte:{[18]]}

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 excepción aquí. Por lo general, desea tragar excepciones en un sitio donde tiene algo significativo que puede hacer en respuesta.

Puedes pensar en try/catch como un mecanismo de transacción. try haciendo estos cambios, si fallan y nos ramificamos en el bloque catch, haga esto (lo que esté en el bloque catch) en respuesta como parte del proceso de reversión y recuperación.

En este caso, simplemente imprimir un stacktrace y luego ser forzado a devolver null no es exactamente una mentalidad de transacción/recuperación. El código transfiere la responsabilidad de manejo de errores a todo el código que llama getClone para comprobar manualmente si hay errores. Es posible que prefiera capturar el CloneNotSupportedException y traducirlo a otra forma más significativa de excepción y lanzarlo, pero no desea simplemente tragar la excepción y devolver un null en este caso, ya que esto no es como un sitio de recuperación de transacciones.

Terminarás filtrando las responsabilidades a las personas que llaman para verificar manualmente y lidiar con el fallo 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. Es posible que tenga un try/catch allí. Durante el proceso de trying para cargar un archivo, puede clonar objetos. Si hay un error en cualquier parte de esta operación de alto nivel (cargando el archivo), normalmente desea lanzar excepciones todo el camino de vuelta a este bloque de transacción de nivel superior try/catch para que pueda recuperarse con gracia de un error en la carga de un archivo (ya sea debido a un error en la clonación o cualquier otra cosa). Así que generalmente no queremos tragarnos una excepción en algún lugar granular como este y luego devolver un null, por ejemplo, ya que eso derrotaría mucho del valor y el propósito de las excepciones. En su lugar, queremos propagar las excepciones todo el camino de regreso a un sitio donde podamos lidiar con ello 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 explicar que no será alcanzado), o hay una mejor manera de codificar ¿algo como esto para que la declaración de retorno adicional sea 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í la cláusula de captura no debe introducirse nunca. Sin embargo, la sintaxis requiere lanzar algo o devolver un valor. Dado que devolver algo no tiene sentido, se usa un InternalError para indicar una condición de VM severa.