¿Lanzar una excepción es un antipatrón aquí?

Acabo de discutir sobre una elección de diseño después de una revisión de código. Me pregunto cuáles son tus opiniones.

Existe esta Preferencesclase, que es un cubo para pares clave-valor. Los valores nulos son legales (eso es importante). Esperamos que ciertos valores aún no se guarden, y queremos manejar estos casos automáticamente al inicializarlos con un valor predeterminado predefinido cuando se solicite.

La solución discutida utilizó el siguiente patrón (NOTA: este no es el código real, obviamente, está simplificado con fines ilustrativos):

public class Preferences {
    // null values are legal
    private Map<String, String> valuesFromDatabase;

    private static Map<String, String> defaultValues;

    class KeyNotFoundException extends Exception {
    }

    public String getByKey(String key) {
        try {
            return getValueByKey(key);
        } catch (KeyNotFoundException e) {
            String defaultValue = defaultValues.get(key);
            valuesFromDatabase.put(key, defaultvalue);
            return defaultValue;
        }
    }

    private String getValueByKey(String key) throws KeyNotFoundException {
        if (valuesFromDatabase.containsKey(key)) {
            return valuesFromDatabase.get(key);
        } else {
            throw new KeyNotFoundException();
        }
    }
}

Fue criticado como un anti-patrón, abusando de las excepciones para controlar el flujo . KeyNotFoundException- traído a la vida solo para ese caso de uso - nunca se verá fuera del alcance de esta clase.

Esencialmente, son dos métodos jugando con él simplemente para comunicarse entre sí.

La clave que no está presente en la base de datos no es algo alarmante o excepcional: esperamos que esto ocurra cada vez que se agregue una nueva configuración de preferencia, de ahí el mecanismo que la inicializa con gracia con un valor predeterminado si es necesario.

El argumento en contra era que getValueByKey- el método privado - como se define en este momento no tiene forma natural de informar al público sobre el método tanto el valor, y si la clave estaba allí. (Si no fuera así, debe agregarse para que el valor pueda actualizarse).

Volver nullsería ambiguo, ya que nulles un valor perfectamente legal, por lo que no se sabe si eso significaba que la clave no estaba allí, o si había un null.

getValueByKeytendría que devolver algún tipo de a Tuple<Boolean, String>, el bool establecido en verdadero si la clave ya está allí, para que podamos distinguir entre (true, null)y (false, null). (Se outpodría usar un parámetro en C #, pero eso es Java).

¿Es esta una mejor alternativa? Sí, tendrías que definir alguna clase de un solo uso para el efecto de Tuple<Boolean, String>, pero luego nos estamos deshaciendo de él KeyNotFoundException, por lo que ese tipo de equilibrio se equilibra. También estamos evitando la sobrecarga de manejar una excepción, aunque no es importante en términos prácticos: no hay consideraciones de rendimiento de las que hablar, es una aplicación cliente y no es que las preferencias del usuario se recuperen millones de veces por segundo.

Una variación de este enfoque podría usar la guayaba Optional<String>(la guayaba ya se usa en todo el proyecto) en lugar de alguna costumbre Tuple<Boolean, String>, y luego podemos diferenciar entre Optional.<String>absent()"apropiado" null. Sin embargo, todavía se siente hackear, por razones que son fáciles de ver: la introducción de dos niveles de "nulidad" parece abusar del concepto que estaba detrás de la creación de Optionals en primer lugar.

Otra opción sería verificar explícitamente si la clave existe (agregue un boolean containsKey(String key)método y llame getValueByKeysolo si ya hemos afirmado que existe).

Finalmente, uno también podría incorporar el método privado, pero el actual getByKeyes algo más complejo que mi ejemplo de código, por lo que la inclusión en línea lo haría parecer bastante desagradable.

Puedo estar dividiendo pelos aquí, pero tengo curiosidad por saber qué apostarías a ser el más cercano a las mejores prácticas en este caso. No encontré una respuesta en las guías de estilo de Oracle o Google.

¿Usar excepciones como en el ejemplo de código es un antipatrón, o es aceptable dado que las alternativas tampoco son muy limpias? Si es así, ¿en qué circunstancias estaría bien? ¿Y viceversa?

Sí, su colega tiene razón: ese es un código incorrecto. Si un error puede manejarse localmente, entonces debe manejarse de inmediato. No se debe lanzar una excepción y luego manejarla de inmediato.

Esto es mucho más limpio que su versión ( getValueByKey()se elimina el método):

public String getByKey(String key) {
    if (valuesFromDatabase.containsKey(key)) {
        return valuesFromDatabase.get(key);
    } else {
        String defaultValue = defaultValues.get(key);
        valuesFromDatabase.put(key, defaultvalue);
        return defaultValue;
    }
}

Se debe lanzar una excepción solo si no sabe cómo resolver el error localmente.

No llamaría a este uso de Excepciones un antipatrón, simplemente no es la mejor solución al problema de comunicar un resultado complejo.

La mejor solución (suponiendo que todavía esté en Java 7) sería usar Guava's Opcional; No estoy de acuerdo con que su uso en este caso sería hackeo. Me parece, basado en la explicación extendida de Opcional de Guava , que este es un ejemplo perfecto de cuándo usarlo. Está diferenciando entre "no se encontró ningún valor" y "valor de nulo encontrado".

Como no hay consideraciones de rendimiento y es un detalle de implementación, en última instancia no importa qué solución elija. Pero tengo que aceptar que es un mal estilo; la clave ausente es algo que sabe que sucederá, y ni siquiera lo maneja más de una llamada a la pila, que es donde las excepciones son más útiles.

El enfoque de tupla es un poco confuso porque el segundo campo no tiene sentido cuando el booleano es falso. Comprobar si la clave existe de antemano es una tontería porque el mapa está buscando la clave dos veces. (Bueno, ya lo estás haciendo, así que en este caso tres veces). La Optionalsolución es perfecta para el problema. Puede parecer un poco irónico almacenar un nullen un Optional, pero si eso es lo que el usuario quiere hacer, no puede decir que no.

Como señaló Mike en los comentarios, hay un problema con esto; Ni Guava ni Java 8 Optionalpermiten almacenar nulls. Por lo tanto, necesitaría rodar el suyo, lo que, aunque es sencillo, implica una buena cantidad de repeticiones, por lo que podría ser excesivo para algo que solo se usará una vez internamente. También puede cambiar el tipo de mapa a Map<String, Optional<String>>, pero el manejo de Optional<Optional<String>>s se vuelve incómodo.

Un compromiso razonable podría ser mantener la excepción, pero reconocer su papel como un "retorno alternativo". Cree una nueva excepción verificada con el seguimiento de la pila deshabilitado, y arroje una instancia pre-creada que pueda mantener en una variable estática. Esto es barato, posiblemente tan barato como una sucursal local , y no puede olvidarse de manejarlo, por lo que la falta de seguimiento de la pila no es un problema.

Existe esta clase de Preferencias, que es un depósito para pares clave-valor. Los valores nulos son legales (eso es importante). Esperamos que ciertos valores aún no se guarden, y queremos manejar estos casos automáticamente al inicializarlos con un valor predeterminado predefinido cuando se solicite.

El problema es exactamente esto. Pero ya publicó la solución usted mismo:

Una variación de este enfoque podría usar el Opcional de Guava (Guava ya se usa en todo el proyecto) en lugar de una Tupla personalizada, y luego podemos diferenciar entre Opcional.absent () y nulo "adecuado" . Sin embargo, todavía se siente hackear, por razones que son fáciles de ver: la introducción de dos niveles de "nulidad" parece abusar del concepto que estaba detrás de la creación de Optionals en primer lugar.

Sin embargo, no use null o Optional . Puede y debe usar Optionalsolo. Para su sensación de "hackear", simplemente utilícela anidada, de modo que termine con lo Optional<Optional<String>>que hace explícito que podría haber una clave en la base de datos (primera capa de opciones) y que podría contener un valor predefinido (segunda capa de opciones).

Este enfoque es mejor que usar excepciones y es bastante fácil de entender siempre que Optionalno sea nuevo para usted.

También tenga en cuenta que Optionaltiene algunas funciones de comodidad, por lo que no tiene que hacer todo el trabajo usted mismo. Éstas incluyen:

• static static <T> Optional<T> fromNullable(T nullableReference)para convertir la entrada de su base de datos a los Optionaltipos
• abstract T or(T defaultValue) para obtener el valor clave de la capa de opción interna o (si no está presente) obtener su valor clave predeterminado

Sé que llego tarde a la fiesta, pero de todos modos su caso de uso se asemeja a cómo JavaProperties permite definir un conjunto de propiedades predeterminadas, que se comprobarán si la instancia no carga ninguna clave correspondiente.

Mirando cómo se realiza la implementación Properties.getProperty(String)(desde Java 7):

Object oval = super.get(key);
String sval = (oval instanceof String) ? (String)oval : null;
return ((sval == null) && (defaults != null)) ? defaults.getProperty(key) : sval;

Realmente no hay necesidad de "abusar de las excepciones para controlar el flujo", como usted ha citado.

Un enfoque similar, pero un poco más conciso, a la respuesta de @ BЈовић también puede ser:

public String getByKey(String key) {
    if (!valuesFromDatabase.containsKey(key)) {
        valuesFromDatabase.put(key, defaultValues.get(key));
    }
    return valuesFromDatabase.get(key);
}

Aunque creo que la respuesta de @BЈовић está bien en caso de que no getValueByKeyse necesite en ningún otro lugar, no creo que su solución sea mala en caso de que su programa contenga ambos casos de uso:

• recuperación por clave con creación automática en caso de que la clave no exista previamente, y

• recuperación sin ese automatismo, sin cambiar nada en la base de datos, repositorio o mapa clave (piense en getValueByKeyser público, no privado)

Si esta es su situación, y siempre que el impacto en el rendimiento sea aceptable, creo que su solución propuesta está totalmente bien. Tiene la ventaja de evitar la duplicación del código de recuperación, no se basa en marcos de terceros y es bastante simple (al menos en mi opinión).

De hecho, en tal situación, depende del contexto si una clave faltante es una situación "excepcional" o no. Para un contexto donde está, getValueByKeyse necesita algo así . Para un contexto donde se espera la creación automática de claves, proporcionar un método que reutilice la función ya disponible, trague la excepción y proporcione un comportamiento de falla diferente, tiene mucho sentido. Esto puede interpretarse como una extensión o "decorador" de getValueByKey, no tanto como una función en la que se abusa de la "excepción por flujo de control".

Por supuesto, hay una tercera alternativa: crear un tercer método privado que devuelva el Tuple<Boolean, String>, como sugirió, y reutilizar este método en getValueByKeyy getByKey. Para un caso más complicado que podría ser la mejor alternativa, pero para un caso tan simple como se muestra aquí, en mi humilde opinión huele a ingeniería excesiva, y dudo que el código sea realmente más fácil de mantener de esa manera. Estoy aquí con la respuesta más importante aquí por Karl Bielefeldt :

"No debería sentirse mal por usar excepciones cuando simplifica su código".

OptionalEs la solución correcta. Sin embargo, si lo prefiere, hay una alternativa que tiene menos sensación de "dos nulos", considere un centinela.

Defina una cadena estática privada keyNotFoundSentinel, con un valor new String(""). *

Ahora el método privado puede en return keyNotFoundSentinellugar de throw new KeyNotFoundException(). El método público puede verificar ese valor

String rval = getValueByKey(key);
if (rval == keyNotFoundSentinel) { // reference equality, not string.equals
    ... // generate default value
} else {
    return rval;
}

En realidad, nulles un caso especial de un centinela. Es un valor centinela definido por el lenguaje, con algunos comportamientos especiales (es decir, comportamiento bien definido si llama a un método null). Resulta que es tan útil tener un centinela como ese que casi todos los idiomas lo usan.

* Usamos intencionalmente en new String("")lugar de limitarnos ""a evitar que Java "interne" la cadena, lo que le daría la misma referencia que cualquier otra cadena vacía. Debido a este paso adicional, estamos garantizados de que la Cadena a la que se hace referencia keyNotFoundSentineles una instancia única, que debemos asegurarnos de que nunca pueda aparecer en el mapa.

Aprenda del marco que aprendió de todos los puntos débiles de Java:

.NET proporciona dos soluciones mucho más elegantes para este problema, ejemplificadas por:

• Dictionary<TKey, TValue>.TryGetValue(TKey, out TValue)
• Nullable<T>.GetValueOrDefault(T default)

Este último es muy fácil de escribir en Java, el primero solo requiere una clase auxiliar de "referencia fuerte".