Cosa c'è di sbagliato in questo design? Sovrascrittura o sovraccarico di java.util.HashMap

Question

Questa domanda mi è stata posta nell'intervista MS. Voglio sapere l'esatto problema di design in questo pezzo di codice. Il codice era già stato dato, necessario per trovare il problema del design.

Ho classe MyHashMap che estende la classe Jash HashMap. Nella classe MyHashMap devo conservare alcune informazioni dei dipendenti. La chiave in questa mappa sarà firstName + lastName + Address.

public MyHashMap extends HashMap<Object, Object> { 
    //some member variables 
    // 
    public void put(String firstName, String lastName, String Address, Object obj) { 
     String key = firstName + lastName+ Address; 
     put(key, obj); 
    } 

    public Object get(String firstName, String lastName, String Address) { 
     String key = firstName + lastName+ Address; 
     return get(key); 
    } 

    public void remove(Strig key) { 
     put(key, ""); 
    } 

    //some more methods 
} 

Cosa c'è di sbagliato in questo design? Devo sottoclassi HashMap o dovrei dichiarare HashMap come variabile membro di questa classe? O dovrei aver implementato i metodi hashCode/Equals?

Answer 1

Ci sono alcuni problemi, ma il problema più grande che posso vedere è che si sta utilizzando un String concatenato come chiave. Le due chiamate seguenti sono diverse, ma equivalenti:

final MyHashMap map = new MyHashMap(); 

map.put("foo", "", "baz", new Object()); 
map.put("", "foo", "baz", new Object()); // Overwrites the previous call 

C'è anche un problema che si sta dichiarando che il tipo di chiave come Object, ma sempre utilizzando String e quindi non sono approfittando della sicurezza di tipo che viene con i generici. Ad esempio, se si desidera eseguire il ciclo attraverso lo keySet del proprio Map, è necessario eseguire il cast di ogni voce Set a String, ma non si è certi che qualcuno non abbia abusato di te Map utilizzando una chiave Integer, per esempio.

Personalmente, vorrei composizione favore per l'eredità Se non avete una buona ragione per non farlo. Nel tuo caso, è MyHashMapsovraccarico i normali metodi di Mapput, get e remove, ma non override nessuno di loro. Dovresti ereditare da una classe per cambiare il suo comportamento, ma la tua implementazione non lo fa, quindi la composizione è una scelta chiara.

di agire come un esempio, un sovraccarico e non viene sostituito significa che se si effettua la seguente dichiarazione:

Map<Object, Object> map = new MyHashMap(); 

nessuno dei vostri metodi dichiarati saranno disponibili. Come raccomandato da alcune delle altre risposte, sarebbe molto meglio utilizzare un oggetto composto da firstName, lastName e indirizzo per agire come chiave della mappa, ma è necessario ricordare di implementare equals e hashCode, altrimenti i valori non saranno recuperabili da il HashMap.

Answer 2

Vorrei dichiarare HashMap come variabile membro della classe.

Non ricordo se una buona spiegazione viene fornita in Codice pulito o in Java efficace, ma in fondo, è più semplice da fare, richiede meno lavoro se l'API cambia.

In generale, estendere tale classe significa che si desidera modificare il suo comportamento.

Answer 3

Ciò che è sbagliato in questo progetto è principalmente il fatto di essere uno HashMap<Oject, Object>, ma non lo è. I metodi sovraccaricati "sostituiscono" i metodi Map, ma quelli sono ancora accessibili - ora dovresti usare la classe in modo incompatibile con l'interfaccia Map e ignorare il modo (ancora tecnicamente possibile) compatibile per usarlo.

Il modo migliore per farlo sarebbe quello di fare una classe EmployeeData con nome e indirizzo campi e hashCode() e equals() metodi basati su quelli (o, meglio ancora, di un campo ID univoco). Quindi non hai bisogno di una sottoclasse Map non standard - puoi semplicemente usare uno HashMap<EmployeeData, Object>. In realtà, il tipo di valore dovrebbe essere più specifico di Object.

Answer 4

mio primo ciak sarebbe che:

public MyHashMap extends HashMap<Oject, Object> 

è sbagliato. Nessun tipo di sicurezza.

se una mappa è richiesto allora almeno renderlo

public MyHashMap extends HashMap<NameAddress, Employee> 

e regolare la put e ottenere lo stesso. Un NameAddress usa nome, cognome e indirizzo negli uguali e hashCode.

Personalmente ritengo che un'interfaccia Set possa corrispondere meglio, con possibilmente una HashMap come membro. Quindi è possibile definire l'interfaccia come dovrebbe essere:

public EmployeeSet implements Set<Employee> { 
    private static class NameAddress { 
    public boolean equals(); 
    public int hashCode(); 
    } 

    private HashMap employees = new HashMap<NameAddress, Employee>(); 

    public add(Employee emp) { 
    NameAddress nad = new NameAddress(emp); 
    empoyees.add(nad, emp); 
    } 

    public remove(Employee emp) { 
    } 
} 

ed estrarre il nome e le informazioni di indirizzo all'interno dell'implementazione per creare la chiave.

EDIT David ha sottolineato che NameAddress non ha bisogno di essere comparabile e ho rimosso quella parte dell'interfaccia. Aggiunto hashCode per correttezza.

Answer 5

Penso che dipenda molto dal contesto e da quello che hanno chiesto esattamente. Ad esempio, è un contesto altamente concorrente, dovresti invece usare Hashtable. Inoltre, è necessario specificare i tipi (String, Object) anziché (Object, Object) per ottenere il supporto del compilatore sulle chiavi.

Penso che un progetto migliore sarebbe stato quello di implementare l'interfaccia Mappa e mantenere HashMap/HashTable come parametro interno dell'oggetto.

Answer 6

La mappa utilizza una chiave interna basata sul nome, il cognome e l'indirizzo. Quindi il metodo remove dovrebbe (1) essere implementato come remove(String firstName, String lastName, String Address) e (2) non dovrebbe modificare il comportamento del metodo di rimozione originale (che ha veramente cancellato la voce) semplicemente cambiando il valore. Una migliore applicazione della remove sarebbe:

public void remove(String firstName, String lastName, String address) { 
    String key = firstName + lastName + address; 
    return remove(key); 
} 

Answer 7

Due altri "crimini efferati" contro le buone pratiche sono che il metodo remove:

• sovraccarichi Map.remove(<K>) e non viene sostituito esso (in modo da avere un'astrazione che perde) e

• implementa un comportamento che è incompatibile con il comportamento del metodo che sovrascrive!

Impostazione del valore associato con una chiave su una stringa vuota non è la stessa come la rimozione della voce per la chiave. (Anche impostarlo su null è leggermente diverso ...)

Questa non è una violazione del principio di sostituibilità di Liskov, ma potrebbe essere davvero fonte di confusione per qualcuno che cerca di utilizzare il tipo. (A seconda che abbiate chiamato il metodo tramite il tipo Map o il tipo MyHashMap, potreste finire per utilizzare metodi diversi con semantica diversa ...)