Lançar uma exceção é um anti-padrão aqui?

Acabei de discutir sobre uma opção de design após uma revisão de código. Eu me pergunto quais são suas opiniões.

Existe essa Preferencesclasse, que é um balde para pares de valores-chave. Valores nulos são legais (isso é importante). Esperamos que determinados valores ainda não sejam salvos e queremos lidar com esses casos automaticamente, inicializando-os com o valor padrão predefinido quando solicitado.

A solução discutida usou o seguinte padrão (NOTA: este não é o código real, obviamente - é simplificado para fins 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();
        }
    }
}

Foi criticado como um antipadrão - abusando de exceções para controlar o fluxo . KeyNotFoundException- trazido à vida apenas para esse caso de uso - nunca será visto fora do escopo desta classe.

São essencialmente dois métodos que o buscam, apenas para comunicar algo um ao outro.

A chave que não está presente no banco de dados não é algo alarmante ou excepcional - esperamos que isso ocorra sempre que uma nova configuração de preferência for adicionada, portanto, o mecanismo que a inicializa normalmente com um valor padrão, se necessário.

O contra-argumento é que getValueByKey- o método privado - conforme definido no momento não tem nenhuma maneira natural de informar o método público sobre tanto o valor, e se a chave estava lá. (Caso contrário, ele deve ser adicionado para que o valor possa ser atualizado).

Retornar nullseria ambíguo, já que nullé um valor perfeitamente legal, portanto não há como dizer se isso significava que a chave não estava lá ou se havia um null.

getValueByKeyteria que retornar algum tipo de a Tuple<Boolean, String>, o bool definido como true se a chave já estiver lá, para que possamos distinguir entre (true, null)e (false, null). (Um outparâmetro pode ser usado em C #, mas isso é Java).

Esta é uma alternativa melhor? Sim, você teria que definir alguma classe de uso único para o efeito de Tuple<Boolean, String>, mas então estamos nos livrando KeyNotFoundException, para que esse tipo de equilíbrio seja equilibrado. Também estamos evitando a sobrecarga de lidar com uma exceção, embora não seja significativa em termos práticos - não há considerações de desempenho, é um aplicativo cliente e não é como se as preferências do usuário fossem recuperadas milhões de vezes por segundo.

Uma variação dessa abordagem poderia usar o Goiaba Optional<String>(o Goiaba já é usado em todo o projeto) em vez de algum costume Tuple<Boolean, String>, e então podemos diferenciar entre Optional.<String>absent()"adequado" null. No entanto, ele ainda parece hackeado por razões fáceis de ver - a introdução de dois níveis de "nulidade" parece abusar do conceito que estava por trás da criação de Optionals em primeiro lugar.

Outra opção seria verificar explicitamente se a chave existe (adicione um boolean containsKey(String key)método e chame getValueByKeyapenas se já tivermos afirmado que ela existe).

Por fim, também é possível incorporar o método privado, mas o real getByKeyé um pouco mais complexo do que o meu exemplo de código, portanto, o inlining o faria parecer bastante desagradável.

Talvez eu esteja dividindo os cabelos aqui, mas estou curioso para saber o que você apostaria para estar mais próximo das melhores práticas neste caso. Não encontrei uma resposta nos guias de estilo da Oracle ou do Google.

O uso de exceções, como no exemplo de código, é um antipadrão ou é aceitável, pois as alternativas também não são muito limpas? Se for, em que circunstâncias seria bom? E vice versa?

Sim, seu colega está certo: esse é um código incorreto. Se um erro puder ser tratado localmente, deverá ser tratado imediatamente. Uma exceção não deve ser lançada e tratada imediatamente.

Isso é muito mais limpo que sua versão (o getValueByKey()método foi removido):

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

Uma exceção deve ser lançada apenas se você não souber como resolver o erro localmente.

Eu não chamaria esse uso de exceções de antipadrão, mas não a melhor solução para o problema de comunicar um resultado complexo.

A melhor solução (supondo que você ainda esteja no Java 7) seria usar o Guava's Optional; Eu discordo que seu uso neste caso seria um hack. Parece-me, com base na extensa explicação do Opcional da Guava , que este é um exemplo perfeito de quando usá-lo. Você está diferenciando entre "nenhum valor encontrado" e "valor de nulo encontrado".

Como não há considerações de desempenho e é um detalhe de implementação, em última análise, não importa qual solução você escolher. Mas tenho que concordar que é um estilo ruim; a chave estar ausente é algo que você sabe que acontecerá, e você nem lida com isso mais de uma chamada na pilha, que é onde as exceções são mais úteis.

A abordagem da tupla é um pouco hacky porque o segundo campo não faz sentido quando o booleano é falso. Verificar se a chave existe de antemão é bobagem, porque o mapa está pesquisando a chave duas vezes. (Bem, você já está fazendo isso, portanto, neste caso, três vezes.) A Optionalsolução é perfeita para o problema. Pode parecer um pouco irônico armazenar um nullem um Optional, mas se é isso que o usuário quer fazer, você não pode dizer não.

Como observado por Mike nos comentários, há um problema com isso; nem o Goava nem o Java 8 Optionalpermitem armazenar nulls. Assim, você precisaria criar o seu próprio que, embora seja direto, envolva uma quantidade razoável de clichê, portanto pode ser um exagero para algo que só será usado uma vez internamente. Você também pode alterar o tipo do mapa para Map<String, Optional<String>>, mas o manuseio de Optional<Optional<String>>s fica estranho.

Um compromisso razoável pode ser manter a exceção, mas reconhecer seu papel como um "retorno alternativo". Crie uma nova exceção verificada com o rastreamento de pilha desativado e lance uma instância pré-criada dela, que você pode manter em uma variável estática. Isso é barato - possivelmente tão barato quanto uma filial local - e você não pode esquecer de lidar com isso, portanto, a falta de rastreamento de pilha não é um problema.

Existe essa classe Preferences, que é um depósito para pares de valores-chave. Valores nulos são legais (isso é importante). Esperamos que determinados valores ainda não sejam salvos e queremos lidar com esses casos automaticamente, inicializando-os com o valor padrão predefinido quando solicitado.

O problema é exatamente isso. Mas você já postou a solução:

Uma variação dessa abordagem pode usar o Opcional do Guava (o Guava já é usado em todo o projeto) em vez de alguma Tupla personalizada e , em seguida, podemos diferenciar entre Optional.absent () e nulo "adequado" . No entanto, ele ainda parece hackeado por razões fáceis de ver - a introdução de dois níveis de "nulidade" parece abusar do conceito que estava por trás da criação de opcionais em primeiro lugar.

No entanto, não use null ou Optional . Você pode e deve usar Optionalapenas. Para o seu sentimento de "hack", basta usá-lo aninhado, para que você Optional<Optional<String>>explique que pode haver uma chave no banco de dados (primeira camada de opção) e que ele pode conter um valor predefinido (segunda camada de opção).

Essa abordagem é melhor do que usar exceções e é muito fácil entender, desde que isso Optionalnão seja novo para você.

Observe também que Optionalpossui algumas funções de conforto, para que você não precise fazer todo o trabalho sozinho. Esses incluem:

• static static <T> Optional<T> fromNullable(T nullableReference)para converter sua entrada do banco de dados nos Optionaltipos
• abstract T or(T defaultValue) para obter o valor-chave da camada de opção interna ou (se não estiver presente) obter seu valor-chave padrão

Sei que estou atrasado para a festa, mas de qualquer maneira seu caso de uso se assemelha ao modo como o Java tambémProperties permite definir um conjunto de propriedades padrão, que será verificado se não houver uma chave correspondente carregada pela instância.

Examinando como a implementação é feita Properties.getProperty(String)(a partir do 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 não há necessidade de "abusar de exceções para controlar o fluxo", como você citou.

Uma abordagem semelhante, mas um pouco mais concisa, à resposta de @ BЈовић também pode ser:

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

Embora eu ache que a resposta de BЈовић é boa, caso não getValueByKeyseja necessária em nenhum outro lugar, não acho que sua solução seja ruim, caso seu programa contenha os dois casos de uso:

• recuperação por chave com criação automática, caso a chave não exista anteriormente e

• recuperação sem esse automatismo, sem alterar nada no banco de dados, repositório ou mapa de chaves (pense em ser getValueByKeypúblico, não privado)

Se essa for a sua situação, e enquanto o desempenho for aceitável, acho que a solução proposta está totalmente correta. Ele tem a vantagem de evitar a duplicação do código de recuperação, não depende de estruturas de terceiros e é bastante simples (pelo menos aos meus olhos).

De fato, em uma situação como essa, depende do contexto se uma chave ausente é uma situação "excepcional" ou não. Para um contexto em que está, algo como getValueByKeyé necessário. Para um contexto em que a criação automática de chave é esperada, fornecer um método que reutilize a função já disponível, traga a exceção e forneça um comportamento de falha diferente, faz todo o sentido. Isso pode ser interpretado como uma extensão ou "decorador" de getValueByKey, não tanto como uma função em que a "exceção é abusada pelo fluxo de controle".

Obviamente, existe uma terceira alternativa: crie um terceiro método privado retornando o Tuple<Boolean, String>, como você sugeriu, e reutilize esse método em getValueByKeye getByKey. Para um caso mais complicado que pode ser de fato a melhor alternativa, mas para um caso tão simples como mostrado aqui, isso tem IMHO com cheiro de superengenharia, e duvido que o código seja realmente mais sustentável dessa maneira. Estou aqui com a melhor resposta aqui de Karl Bielefeldt :

"você não deve se sentir mal ao usar exceções quando isso simplifica seu código".

Optionalé a solução correta. No entanto, se você preferir, há uma alternativa com menos sensação de "dois nulos", considere uma sentinela.

Defina uma sequência estática privada keyNotFoundSentinel, com um valor new String(""). *

Agora, o método privado pode, em return keyNotFoundSentinelvez de throw new KeyNotFoundException(). O método público pode verificar esse valor

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

Na realidade, nullé um caso especial de sentinela. É um valor sentinela definido pelo idioma, com alguns comportamentos especiais (ou seja, comportamento bem definido se você chamar um método null). Por acaso é tão útil ter um sentinela como esse que quase toda a linguagem o usa.

* Utilizamos intencionalmente, new String("")e não apenas, ""para impedir que o Java "interne" a string, o que daria a mesma referência que qualquer outra string vazia. Como fizemos essa etapa extra, temos a garantia de que a String mencionada por keyNotFoundSentinelé uma instância única, da qual precisamos garantir que ela nunca apareça no próprio mapa.

Aprenda com a estrutura que aprendeu com todos os pontos problemáticos do Java:

O .NET fornece duas soluções muito mais elegantes para esse problema, exemplificadas por:

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

O último é muito fácil de escrever em Java, o primeiro requer apenas uma classe auxiliar de "referência forte".