example - my icon java

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

Question

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 nula (y supongamos que sabemos que la clase a la que llamamos admite la clonación) para que sepamos que la declaración de prueba 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 explica que no se alcanzará), o hay una mejor manera de codificar algo como esto para que el extra declaración de devolución es innecesaria?

Answer 1

¿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 explica 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) para llegar a esa línea, algo salió mal y la aplicación está en un estado desconocido. La mayoría de esto es (como arriba) porque el desarrollador ha perdido algo (los objetos pueden ser nulos y no clonables).

Probablemente no usaría InternalError menos que esté 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 arroje esa excepción.

Answer 2

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

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

Sin embargo, para responder a la pregunta, ¡los programas tipo Lint seguramente no lo han resuelto! He visto a dos diferentes luchar 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 el descanso era una violación 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 el descanso agregado, luego eliminado y luego agregado y eliminado, según el analizador de código que ejecutaran ese día.

Si termina en esta situación, elija una y comente la anomalía, que es la buena forma que se mostró. Esa es la mejor y más importante comida para llevar.

Answer 3

El retorno nulo; es necesario ya que se puede detectar una excepción, sin embargo, en tal caso ya que ya verificamos si era nula (y supongamos que sabemos que la clase a la que llamamos admite la clonación) para que sepamos que la declaración de prueba nunca fallará.

Si conoce 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? Evite el try si sabe con certeza que las cosas siempre tendrán é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 mente. Ve la función y sus entradas, y dada la información que tiene, necesita esa declaración de 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 que explica que no se alcanzará), o hay una mejor manera de codificar algo como esto para que el extra declaración de devolución es innecesaria?

Todo lo contrario, sugeriría 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 recuento de líneas aquí. Establezca la confiabilidad de la función a través de pruebas y luego continúe. Simplemente fingiendo que puede omitir la declaración de return , imagine volver a ese código un año después y luego intente decidir si esa declaración de return en la parte inferior causará más confusión que algún comentario que detalla las minucias de por qué se omitió debido a suposiciones puede hacer acerca de los parámetros de entrada. Lo más probable es que la declaración de return 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í. En general, desea tragar excepciones en un sitio donde tiene algo significativo que puede hacer en respuesta.

Puede pensar en try/catch como un mecanismo de transacción. try realizar estos cambios, si fallan y nos ramificamos en el bloque catch , haga esto (lo que sea que esté en el bloque catch ) 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 nulo 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 a getClone para verificar manualmente las fallas. Es posible que prefiera capturar la CloneNotSupportedException y traducirla a otra forma de excepción más significativa y lanzarla, pero no desea simplemente tragar la excepción y devolver un valor nulo en este caso, ya que esto no es como un sitio de recuperación de transacciones .

Terminará filtrando las responsabilidades a las personas que llaman para verificar manualmente y lidiar con 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 try/catch allí. Durante el proceso de trying cargar un archivo, puede clonar objetos. Si hay una falla en alguna parte de esta operación de alto nivel (cargando el archivo), por lo general, desea lanzar excepciones a este bloque de try/catch 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 simplemente tragar una excepción en un lugar granular como este y luego devolver un valor nulo, por ejemplo, ya que eso anularía mucho el valor y el propósito de las excepciones. En su lugar, queremos propagar excepciones hasta un sitio donde podamos tratarlo de manera significativa.

Answer 4

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

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

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

Tomemos, por ejemplo, el método Scanner#ioException :

Devuelve la última IOException lanzada por el Lector subyacente de este escáner. Este método devuelve nulo si no existe tal excepción .

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

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

Un valor de retorno de nulo no necesariamente indica que el mapa no contiene mapeo para la clave; También es posible que el mapa asigne explícitamente la clave a nulo . La operación usesKey se puede usar 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.

Answer 5

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, usted 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 usted reclama nunca se ejecutará. ¿Por qué?

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

Por cierto, tu código no se compilará para mí. No puede 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

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 realmente necesita que el tipo de parámetro sea Cloneable , lo siguiente al menos se compila.

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

Answer 6

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

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

No hay ningún beneficio en verificar a para ver si es nulo. 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 es beneficioso desechar el código con pruebas nulas inútiles y manejo de excepciones inútil. 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; es por eso que se necesitaba el retorno. El OP no se habría equivocado con el método que propongo).

Answer 7

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 los bloques de try por esa misma razón.

Answer 8

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 sintaxis o semántica, no tiene otra opción al respecto.

Answer 9

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

Y para evitar excepciones marcadas, volvería a lanzar CloneNotSupportedException como RuntimeException .

Para ambos, puede agregar un texto agradable con la intención de que esto no ocurra o sea el caso.

public Object getClone(Object a) {
Objects.requireNonNull(a);
try {
return a.clone();
} catch (CloneNotSupportedException e) {
throw new IllegalArgumentException(e);
}
}

Answer 10

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

Answer 11

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 explica que no se alcanzará), o hay una mejor manera de codificar algo como esto para que el extra declaración de devolución es innecesaria?

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

 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 catch nunca debe ingresarse. Aún así, la sintaxis requiere arrojar algo o devolver un valor. Como devolver algo no tiene sentido, se utiliza un InternalError para indicar una condición grave de VM.

Answer 12

Una forma más clara sin una declaración de devolución adicional es la siguiente. Tampoco captaría CloneNotSupportedException , pero lo dejo 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 directa que la que tiene inicialmente.

Answer 13

CloneNotSupportedException que significa que su código puede manejarlo. Pero después de atraparlo, literalmente no tiene idea de qué hacer cuando llega al final de la función, lo que implica que no puede manejarlo. Entonces tiene razón en que es un olor a código en este caso, y en mi opinión significa que no debería haber captado CloneNotSupportedException .