sta lanciando un'eccezione anti-pattern qui?

32

Ho appena avuto una discussione su una scelta di design dopo una revisione del codice. Mi chiedo quali sono le tue opinioni.

Esiste questa classe Preferences , che è un bucket per le coppie chiave-valore. I valori nulli sono legali (questo è importante). Prevediamo che alcuni valori potrebbero non essere ancora salvati e vogliamo gestire questi casi automaticamente inizializzandoli con il valore predefinito predefinito quando richiesto.

La soluzione discussa usata seguendo il modello (NOTA: questo è non il codice reale, ovviamente - è semplificato a scopo illustrativo):

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

È stato criticato come anti-pattern - abuso di eccezioni per controllare il flusso . KeyNotFoundException - portato alla vita solo per quel solo caso d'uso - non verrà mai visto al di fuori dello scopo di questa classe.

Sono essenzialmente due metodi che giocano a fetch semplicemente per comunicare qualcosa l'uno con l'altro.

La chiave non presente nel database non è qualcosa di allarmante, o eccezionale - ci aspettiamo che ciò accada ogni volta che viene aggiunta una nuova impostazione di preferenza, quindi il meccanismo che lo inizializza con grazia con un valore predefinito, se necessario.

Il controargomento era che getValueByKey - il metodo privato - come definito in questo momento non ha alcun modo naturale di informare il metodo pubblico su entrambi il valore e se la chiave era lì. (In caso contrario, deve essere aggiunto in modo che il valore possa essere aggiornato).

Restituire null sarebbe ambiguo, poiché null è un valore perfettamente legale, quindi non si può sapere se significasse che la chiave non era presente, o se c'era un null .

getValueByKey dovrebbe restituire una sorta di Tuple<Boolean, String> , il bool è impostato su true se la chiave è già presente, in modo da poter distinguere tra (true, null) e (false, null) . (Un parametro out potrebbe essere usato in C #, ma quello è Java).

Questa è un'alternativa migliore? Sì, dovresti definire alcune classi monouso con l'effetto di Tuple<Boolean, String> , ma poi ci sbarazzeremo di KeyNotFoundException , quindi quel tipo di saldo. Stiamo anche evitando il sovraccarico di gestione di un'eccezione, sebbene non sia significativa in termini pratici - non ci sono considerazioni sulle prestazioni per parlare, è un'app client e non è come se le preferenze dell'utente vengano recuperate milioni di volte al secondo.

Una variante di questo approccio potrebbe utilizzare Optional<String> di Guava (Guava è già utilizzato in tutto il progetto) invece di Tuple<Boolean, String> personalizzato, quindi possiamo distinguere tra Optional.<String>absent() e "proper" null . Si sente comunque un hacker, tuttavia, per ragioni che sono facili da vedere - l'introduzione di due livelli di "nullità" sembra abusare del concetto che stava dietro creando Optional s in primo luogo.

Un'altra opzione sarebbe quella di verificare esplicitamente se la chiave esiste (aggiungi un metodo boolean containsKey(String key) e chiama getValueByKey solo se abbiamo già affermato che esiste).

Infine, si potrebbe anche inline il metodo privato, ma il getByKey effettivo è un po 'più complesso del mio esempio di codice, quindi l'inlining lo renderebbe piuttosto brutto.

Potrei essermi diviso i capelli qui, ma sono curioso di sapere cosa scommetterei per essere il più vicino alle migliori pratiche in questo caso. Non ho trovato una risposta nelle guide di stile di Oracle o di Google.

L'uso di eccezioni come nel codice di esempio è un anti-pattern, o è accettabile, dato che anche le alternative non sono molto pulite? Se lo è, in quali circostanze andrebbe bene? E viceversa?

    
posta Konrad Morawski 12.02.2015 - 16:48
fonte

8 risposte

73

Sì, il tuo collega ha ragione: questo è un codice cattivo. Se un errore può essere gestito localmente, allora dovrebbe essere gestito immediatamente. Un'eccezione non dovrebbe essere lanciata e quindi gestita immediatamente.

Questo è molto più pulito della tua versione (il metodo getValueByKey() è stato rimosso):

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

Un'eccezione dovrebbe essere lanciata solo se non sai come risolvere localmente l'errore.

    
risposta data 12.02.2015 - 17:01
fonte
11

Non definirei questo uso di Exceptions un anti-pattern, ma non la migliore soluzione al problema della comunicazione di un risultato complesso.

La soluzione migliore (supponendo che tu sia ancora su Java 7) sarebbe usare Opava di Guava; Non sono d'accordo sul fatto che il suo uso in questo caso sarebbe hacker. Mi sembra che, in base alla spiegazione estesa di Guava di Optional , questo sia un perfetto esempio di quando usarlo. Stai distinguendo tra "nessun valore trovato" e "valore di null trovato".

    
risposta data 12.02.2015 - 17:11
fonte
8

Poiché non ci sono considerazioni sulle prestazioni ed è un dettaglio di implementazione, alla fine non importa quale soluzione scegli. Ma devo dire che è cattivo stile; la chiave assente è qualcosa che sai che succederà e che non lo gestisci nemmeno più di una chiamata nello stack, che è dove le eccezioni sono più utili.

L'approccio tuple è un po 'hacky perché il secondo campo non ha senso quando il booleano è falso. Controllare se la chiave esiste in anticipo è sciocco perché la mappa sta guardando la chiave due volte. (Beh, lo stai già facendo, quindi in questo caso tre volte.) La soluzione Optional è perfetta per il problema. Potrebbe sembrare un po 'ironico archiviare un null in Optional , ma se questo è ciò che l'utente vuole fare, non puoi dire di no.

Come notato da Mike nei commenti, c'è un problema con questo; né Guava né Java 8's Optional consentono di memorizzare null s. Quindi è necessario eseguire il rollover che, pur essendo semplice, richiede una buona quantità di piastre, quindi potrebbe essere eccessivo per qualcosa che verrà utilizzato solo una volta internamente. Puoi anche modificare il tipo di mappa in Map<String, Optional<String>> , ma gestire Optional<Optional<String>> s diventa difficile.

Un ragionevole compromesso potrebbe essere quello di mantenere l'eccezione, ma riconoscere il suo ruolo come "ritorno alternativo". Crea una nuova eccezione controllata con la traccia dello stack disabilitata e lancia una sua istanza creata che puoi mantenere in una variabile statica. Questo è economico - probabilmente economico come una filiale locale - e non puoi dimenticare di gestirlo, quindi il la mancanza di traccia dello stack non è un problema.

    
risposta data 12.02.2015 - 17:33
fonte
5

There's this Preferences class, which is a bucket for key-value pairs. Null values are legal (that's important). We expect that certain values may not be saved yet, and we want to handle these cases automatically by initializing them with predefined default value when requested.

Il problema è esattamente questo. Ma hai già postato la soluzione da solo:

A variation of this approach could use Guava's Optional (Guava is already used throughout the project) instead of some custom Tuple, and then we can differentiate between Optional.absent() and "proper" null. It still feels hackish, though, for reasons that are easy to see - introducing two levels of "nullness" seem to abuse the concept that stood behind creating Optionals in the first place.

Tuttavia, non utilizzare null o Optional . Puoi e dovresti usare solo Optional . Per la tua sensazione di "hacker", basta usarlo nidificato, così si finisce con Optional<Optional<String>> che rende esplicito che potrebbe esserci una chiave nel database (primo livello di opzione) e che potrebbe contenere un valore predefinito (secondo livello di opzione) .

Questo approccio è migliore dell'utilizzo di eccezioni ed è abbastanza facile da capire se Optional non è nuovo per te.

Inoltre, ricorda che Optional ha alcune funzioni di comfort, in modo che tu non debba fare tutto il lavoro da solo. Questi includono:

  • static static <T> Optional<T> fromNullable(T nullableReference) per convertire l'input del tuo database in Optional types
  • abstract T or(T defaultValue) per ottenere il valore chiave del livello opzione interno o (se non presente) ottenere il valore chiave predefinito
risposta data 12.02.2015 - 18:22
fonte
5

So di essere in ritardo per la festa, ma comunque il tuo caso d'uso ricorda come Java Properties consente di definire anche un insieme di proprietà predefinite, che verrà controllato se non vi è alcuna chiave corrispondente caricata dall'istanza.

Osservando come viene eseguita l'implementazione per Properties.getProperty(String) (da Java 7):

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

Non c'è davvero bisogno di "abusare delle eccezioni per controllare il flusso", come hai citato.

Un approccio simile, ma leggermente più conciso, alla risposta di @ BЈовић può anche essere:

public String getByKey(String key) {
    if (!valuesFromDatabase.containsKey(key)) {
        valuesFromDatabase.put(key, defaultValues.get(key));
    }
    return valuesFromDatabase.get(key);
}
    
risposta data 13.02.2015 - 11:04
fonte
4

Anche se penso che la risposta di @ BЈовић vada bene nel caso in cui getValueByKey sia necessaria da nessun'altra parte, non penso che la tua soluzione sia negativa nel caso in cui il tuo programma contenga entrambi i casi d'uso:

  • recupero per chiave con creazione automatica nel caso in cui la chiave non esistesse prima e

  • recupero senza quell'automatismo, senza modificare nulla nel database, nell'archivio o nella mappa chiave (pensa a getValueByKey come pubblico, non privato)

Se questa è la tua situazione, e fintanto che la performance è accettabile, penso che la tua soluzione proposta sia completamente ok. Ha il vantaggio di evitare la duplicazione del codice di recupero, non si basa su framework di terze parti, ed è piuttosto semplice (almeno ai miei occhi).

In effetti, in una situazione del genere, dipende dal contesto se una chiave mancante è o meno una situazione "eccezionale". Per un contesto in cui è, è necessario qualcosa come getValueByKey . Per un contesto in cui è prevista la creazione automatica di chiavi, fornire un metodo che riutilizza la funzione già disponibile, ingoia l'eccezione e fornisce un comportamento di errore diverso, ha perfettamente senso. Questo può essere interpretato come un'estensione o "decoratore" di getValueByKey , non tanto come una funzione in cui "l'eccezione è abusata per il flusso di controllo".

Ovviamente, esiste una terza alternativa: creare un terzo metodo privato restituendo Tuple<Boolean, String> , come suggerito, e riutilizzare questo metodo sia in getValueByKey che getByKey . Per un caso più complicato che potrebbe essere davvero l'alternativa migliore, ma per un caso così semplice come mostrato qui, questo ha un odore di overengineering a IMHO, e dubito che il codice diventi davvero più mantenibile in questo modo. Sono qui con la risposta più alta qui da Karl Bielefeldt :

"you shouldn't feel bad about using exceptions when it simplifies your code".

    
risposta data 12.02.2015 - 18:23
fonte
3

Optional è la soluzione corretta. Tuttavia, se preferisci, c'è un'alternativa che ha meno di una sensazione di "due null", considera una sentinella.

Definisci una stringa statica privata keyNotFoundSentinel , con un valore new String("") . *

Ora il metodo privato può return keyNotFoundSentinel anziché throw new KeyNotFoundException() . Il metodo pubblico può controllare quel valore

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

In realtà, null è un caso speciale di sentinella. È un valore sentinella definito dalla lingua, con alcuni comportamenti speciali (cioè un comportamento ben definito se si chiama un metodo su null ). Capita solo di essere così utile avere una sentinella come quella che quasi mai la usa.

* Utilizziamo intenzionalmente new String("") anziché solo "" per impedire a Java di "internare" la stringa, che gli darebbe lo stesso riferimento di qualsiasi altra stringa vuota. Poiché questo passaggio extra ci ha garantito che la stringa a cui fa riferimento keyNotFoundSentinel è un'istanza univoca, dobbiamo assicurarci che non possa mai apparire nella mappa stessa.

    
risposta data 14.02.2015 - 03:42
fonte
2

Impara dal framework che ha imparato da tutti i punti critici di Java:

.NET offre due soluzioni molto più eleganti a questo problema, esemplificato da:

Quest'ultimo è molto facile da scrivere in Java, il primo richiede solo una classe helper di "riferimento strong".

    
risposta data 12.02.2015 - 21:13
fonte