¿Mala decisión de diseño para lanzar una excepción desde un acceso?

Question

He leído algunas respuestas re los pros y los contras de lanzar una excepción dentro de un descriptor de acceso, pero pensé que iba a abordar mi pregunta específica con un ejemplo:

public class App { 
    static class Test {  
    private List<String> strings; 

    public Test() { 
    } 

    public List<String> getStrings() throws Exception { 
     if (this.strings == null) 
      throw new Exception(); 

     return strings; 
    } 

    public void setStrings(List<String> strings) { 
     this.strings = strings; 
    } 
} 

public static void main(String[] args) { 
    Test t = new Test(); 

    List<String> s = null; 
    try { 
     s = t.getStrings(); 
    } catch (Exception e) { 
     // TODO: do something more specific 
    } 
    } 
} 

getStrings() está lanzando una Exception cuando strings no ha sido conjunto. ¿Esta situación sería mejor manejada por un método?

Answer 1

Sí, eso es malo. Sugiero inicializar la variable de cadenas en la declaración, como:

private List<String> strings = new ArrayList<String>(); 

y se podía sustituir a la incubadora con algo como

public void addToList(String s) { 
    strings.add(s); 
} 

Así que no hay posibilidad de un NPE.

(alternativa no inicializar las cadenas de variables en la declaración, agregue la inicialización con el método addToList, y tienen el código de usarlo compruebe getStrings() para ver si se hace nula la espalda.)

cuanto a por qué es malo, es difícil de decir con un ejemplo tan artificial, pero parece que hay demasiados cambios de estado y penaliza al usuario de esta clase al hacer que el usuario maneje la excepción. También existe el problema de que una vez que los métodos en su programa comiencen a lanzar Exception, esto tiende a hacer metástasis en toda su base de código.

Comprobando que este miembro de instancia se llene debe hacerse en alguna parte, pero creo que debería hacerse en algún lugar que tenga más conocimiento de lo que está sucediendo que en esta clase.

Answer 2

Sugeriría arrojar la excepción del método setter. ¿Hay alguna razón especial por la cual no sería mejor hacerlo?

public List<String> getStrings() throws Exception { 
    return strings; 
} 

public void setStrings(List<String> strings) { 
    if (strings == null) 
     throw new Exception(); 

    this.strings = strings; 
} 

También, dependiendo del contexto particular, ¿no sería posible simplemente pasar el List<String> strings directamente por el constructor? De esa manera tal vez ni siquiera necesitarías un método setter.

Answer 3

Realmente depende de lo que están haciendo con la lista que obtienes. Creo que una solución más elegante sería devolver un Colletions.EMPTY_LIST o, como sugiere @Nathan, un ArrayList vacío. Luego, el código que usa la lista puede verificar si está vacío si lo desea, o simplemente trabajar con él como con cualquier otra lista.

La diferencia es la distinción entre no hay cuerdas y no existe una lista de cadenas. El primero debe estar representado por una lista vacía, el segundo devolviendo null o lanzando una excepción.

Answer 4

La forma correcta de manejar esto, asumiendo que es un requisito que la lista se establece en otro lugar antes llamando al captador, es la siguiente:

public List<String> getStrings() { 
    if (strings == null) 
     throw new IllegalStateException("strings has not been initialized"); 

    return strings; 
} 

Ser un sin control excepción, que ISN' Se requiere que usted lo declare en el método, para que no contamine el código del cliente con mucho ruido de prueba/captura.Además, como esta condición es evitable (es decir, el código del cliente puede garantizar que la lista se haya inicializado), es un error de programación , y por lo tanto, una excepción no verificada es aceptable y preferible.

Answer 5

Me gustaría consultar la especificación Java Beans en esto. Probablemente sea mejor no arrojar una excepción java.lang.Exception, sino usar java.beans.PropertyVetoException y hacer que su bean se registre como java.beans.VetoableChangeListener y desencadenar el evento apropiado. Es un poco exagerado, pero identificaría correctamente lo que estás tratando de hacer.

Answer 6

Realmente depende de lo que estás tratando de hacer. Si intenta hacer cumplir la condición de que se ha llamado a la configuración antes de que se llame al captador, puede haber un caso para lanzar una IllegalStateException.

Answer 7

En general, este es un olor de diseño.

Si realmente es un error que las cadenas no se inicializan, elimine el setter y aplique la restricción en un constructor, o haga lo que dice Bohemian y genere una IllegalArgumentException con un mensaje explicativo. Si haces lo primero, obtienes un comportamiento a prueba de fallas.

Si no es un error, strings es nulo, considere la posibilidad de inicializar la lista en una lista vacía e imponerla para que no sea nula en el setter, de forma similar a la anterior. Esto tiene la ventaja de que no necesita verificar nulo en ningún código de cliente.

for (String s: t.getStrings()) { 
    // no need to check for null 
} 

Esto puede simplificar mucho el código de cliente.

EDITAR: Ok, acabo de ver que estás serializando desde JSON, por lo que no puedes eliminar el constructor. Por lo que usted necesita, ya sea:

1. lanzar una IllegalArgumentException del captador
2. si strings es nulo, devolver un Collections.EMPTY_LIST.
3. o superior: agregue una comprobación de validación justo después de la deserialización en la que compruebe específicamente el valor de las cadenas y genere una excepción razonable.

Answer 8

Creo que ha expuesto una pregunta más importante detrás de la original: ¿debería trabajar para evitar casos excepcionales en getters y setters?

La respuesta es sí, debe trabajar para evitar excepciones triviales.

Lo que tenemos aquí es efectivamente lazy instantiation que no está motivado en absoluto en este ejemplo:

En la programación informática, la inicialización perezosa es la táctica de retrasar la creación de un objeto, el cálculo de un valor , o algún otro proceso costoso hasta la primera vez que sea necesario.

usted tiene dos problemas en este ejemplo:

1. Su ejemplo no es seguro para subprocesos. Esa comprobación de nulo puede tener éxito (es decir, encontrar que el objeto es nulo) en dos hilos al mismo tiempo. Su creación de instancias luego crea dos listas diferentes de cadenas. Comportamiento no determinista se producirá.

2. No hay ninguna buena razón en este ejemplo aplazar la creación de instancias. No es una operación costosa o complicada. Esto es lo que quiero decir con "trabajar para evitar excepciones triviales": vale la pena invertir los ciclos para crear una lista útil (pero vacía) para asegurarse de no lanzar excepciones de puntero nulo de detonación retardada.

Recuerde, cuando infligir una excepción en el código fuera, que está básicamente la esperanza de que los desarrolladores saben cómo hacer algo razonable con él. También estás esperando que no hay un tercer promotor en la ecuación cuya envuelto todo en un comedor de excepción, sólo para ponerse e ignorar excepciones como la suya:

try { 
    // I don't understand why this throws an exception. Ignore. 
    t.getStrings(); 
} catch (Exception e) { 
    // Ignore and hope things are fine. 
} 

En el siguiente ejemplo, estoy usando Null Object pattern para indicar al código futuro que su ejemplo no se ha establecido. Sin embargo, el Objeto Nulo se ejecuta como un código no excepcional y, por lo tanto, no tiene los gastos generales y el impacto en el flujo de trabajo de futuros desarrolladores.

public class App { 
    static class Test {    
     // Empty list 
     public static final List<String> NULL_OBJECT = new ArrayList<String>(); 

     private List<String> strings = NULL_OBJECT; 

     public synchronized List<String> getStrings() { 
      return strings; 
     }  

     public synchronized void setStrings(List<String> strings) {   
      this.strings = strings;  
     } 
    } 

    public static void main(String[] args) {  
     Test t = new Test();  

     List<String> s = t.getStrings(); 

     if (s == Test.NULL_OBJECT) { 
      // Do whatever is appropriate without worrying about exception 
      // handling. 

      // A possible example below: 
      s = new ArrayList<String>(); 
      t.setStrings(s); 
     } 

     // At this point, s is a useful reference and 
     // t is in a consistent state. 
    } 
}