È eccessivo riempire una collezione in una classe semplice solo per una migliore leggibilità?

15

Ho la seguente mappa:

Map<Double, List<SoundEvent>> soundEventCells = new HashMap<Double, List<SoundEvent>>();

Questo HashMap mappa double valori (che sono punti nel tempo) alla corrispondente SoundEvent 'cella': ogni 'cella' può contenere un numero di SoundEvent s. Ecco perché è implementato come List<SoundEvent> , perché è esattamente quello che è.

Per motivi di migliore leggibilità del codice, ho pensato di implementare una classe interna statica molto semplice in questo modo:

private static class SoundEventCell {
    private List<SoundEvent> soundEvents = new ArrayList<SoundEvent>();
    public void addEvent(SoundEvent event){
        soundEvents.add(event);
    }
    public int getSize(){
        return soundEvents.size();
    }
    public SoundEvent getEvent(int index){
        return soundEvents.get(index);
    }
    // .. remove() method unneeded
}

E la dichiarazione della mappa (e molti altri codici) apparirebbe meglio, ad esempio:

Map<Double, SoundEventCell> soundEventCells = new HashMap<Double, SoundEventCell>();

Questo è eccessivo? Lo faresti nei tuoi progetti?

    
posta Aviv Cohn 17.09.2014 - 16:32
fonte

4 risposte

12

Non è affatto eccessivo. Inizia con le operazioni di cui hai bisogno, piuttosto che iniziare con "Posso usare una HashMap". A volte una HashMap è proprio ciò di cui hai bisogno.
Nel tuo caso sospetto che non lo sia. Quello che probabilmente vuoi fare è qualcosa di simile a questo:

public class EventsByTime {
    public EventsByTime addEvent(double time, SoundEvent e);
    public List<SoundEvent> getEvents(double time);
    // ... more methods specific to your use ...
}

Sicuramente non vuoi avere un sacco di codice che dice questo:

List<SoundEvent> events = eventMap.get(time);
if (events == null) {
   events = new ArrayList<SoundEvent>();
   eventMap.put(time, events);
}

O forse potresti semplicemente usare uno dei Guava Multimap implementazioni.

    
risposta data 17.09.2014 - 23:28
fonte
13

Anche se può essere di aiuto nella leggibilità in alcune aree, può anche complicare le cose. Personalmente, mi sto allontanando dal confezionamento o estendendo le raccolte per motivi di scioltezza, poiché il nuovo involucro, in fase di lettura iniziale, mi suggerisce che potrebbero esserci dei comportamenti di cui ho bisogno per essere consapevole. Consideralo un'ombra di Principle of Least Surprise.

Attenersi all'implementazione dell'interfaccia significa che devo solo preoccuparmi dell'interfaccia. L'implementazione concreta può, ovviamente, ospitare un comportamento aggiuntivo, ma non dovrei aver bisogno di preoccuparmene. Quindi, quando cerco di trovare la mia strada attraverso il codice di qualcuno, preferisco le semplici interfacce per la leggibilità.

Se, d'altra parte, trovi un caso d'uso che fa benefici del comportamento aggiunto, allora hai un argomento per migliorare il codice creando una classe completa.

    
risposta data 17.09.2014 - 17:07
fonte
2

Il wrapping limita le tue funzionalità solo a quei metodi che decidi di scrivere, aumentando sostanzialmente il tuo codice senza alcun beneficio. Per lo meno, vorrei provare il seguente:

private static class SoundEventCell : List<SoundEvent>
{
}

Puoi ancora scrivere il codice dal tuo esempio.

Map<Double, SoundEventCell> soundEventCells = new HashMap<Double, SoundEventCell>();

Detto questo, l'ho sempre fatto solo quando ci sono alcune funzionalità di cui necessita la lista stessa. Ma penso che il tuo metodo sarebbe eccessivo per questo. A meno che tu non abbia un motivo per voler limitare l'accesso alla maggior parte dei metodi di List.

    
risposta data 17.09.2014 - 22:15
fonte
-1

Un'altra soluzione potrebbe essere quella di definire la tua classe wrapper con un singolo metodo che espone l'elenco:

private static class SoundEventCell
{
    private List<SoundEvent> events;

    public SoundEventCell(List<SoundEvent> events)
    {
        this.events = events;
    }

    public List<SoundEvent> getEvents()
    {
        return events;
    }
}

Questo ti dà la tua classe ben denominata con il codice minimo, ma ti dà ancora l'incapsulamento, permettendoti di es. rendere la classe immutabile (eseguendo una copia difensiva nel costruttore e utilizzando Collections.unmodifiableList nell'accessor).

(Tuttavia, se questi elenchi vengono effettivamente utilizzati solo in questa classe, penso che farebbe meglio a sostituire il Map<Double, List<SoundEvent>> con un Multimap<Double, SoundEvent> ( docs ), poiché spesso consente di risparmiare un sacco di logica ed errori di controllo nullo.)

    
risposta data 17.09.2014 - 23:14
fonte

Leggi altre domande sui tag