Czy przesada jest zawijanie kolekcji do prostej klasy tylko ze względu na lepszą czytelność?

15

Mam następującą mapę:

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

Ten HashMapmapy doublewartości (które są punktami, w czasie) do odpowiadającej SoundEvent„komórki”: każdy z „komórek” może zawierać kilka SoundEventsekund. Dlatego jest zaimplementowany jako List<SoundEvent>, ponieważ właśnie tak jest.

Ze względu na lepszą czytelność kodu pomyślałem o zaimplementowaniu bardzo prostej statycznej klasy wewnętrznej, takiej jak:

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
}

I wtedy deklaracja mapy (i wiele innych kodów) wyglądałaby lepiej, na przykład:

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

Czy to przesada? Czy zrobiłbyś to w swoich projektach?

Aviv Cohn
źródło
można argumentować, że koncepcyjnie zostało to rozwiązane w Skąd wiesz, czy napisałeś czytelny i łatwy do utrzymania kod? Jeśli twoi rówieśnicy wciąż narzekają na twój sposób robienia rzeczy, czy to w ten czy inny sposób, lepiej się zmień, aby poczuli się lepiej
gnat
1
Co sprawia, że ​​lista zdarzeń dźwiękowych jest „komórką”, a nie listą? Czy ten wybór słów oznacza, że ​​komórka ma lub ostatecznie będzie zachowywać się inaczej niż lista?
Kod x
@DocBrown Dlaczego? Klasa jest private staticdlatego, że będzie używana tylko przez klasę zewnętrzną, ale nie jest związana z żadnym konkretnym wystąpieniem klasy zewnętrznej. Czy to nie jest właściwe użycie private static?
Aviv Cohn
2
@Doc Brown, Aviv Cohn: Nie ma tagu określającego dowolny język, więc wszystko może być jednocześnie dobre i złe!
Emilio Garavaglia,
@EmilioGaravaglia: Java (myślę, że to całkiem jasne, ponieważ sądząc po składni, może to być Java lub C #, a stosowane konwencje zawężają ją do Java;)).
Aviv Cohn

Odpowiedzi:

12

To wcale nie jest przesada. Zacznij od operacji, których potrzebujesz, a nie od „Mogę użyć HashMapy”. Czasami HashMap jest właśnie tym, czego potrzebujesz.
W twoim przypadku podejrzewam, że tak nie jest. To, co prawdopodobnie chcesz zrobić, to coś takiego:

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

Na pewno nie chcesz mieć dużo kodu mówiącego:

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

A może możesz po prostu użyć jednej z implementacji Guava Multimap .

Kevin Cline
źródło
Więc opowiadasz się za wykorzystaniem klasy, która jest zasadniczo mechanizmem ukrywania informacji, jako ... mechanizmem ukrywania informacji? Horror.
Robert Harvey
1
Właściwie tak, mam TimeLineklasę właśnie do takich rzeczy :) Jest to cienkie opakowanie wokół HashMap<Double, SoundEventCell>(ostatecznie wpadłem na pomysł SoundEventCellzamiast tego List<SoundEvent>). Mogę po prostu zrobić timeline.addEvent(4.5, new SoundEvent(..))i zapisać rzeczy niższego poziomu :)
Aviv Cohn
14

Chociaż może to poprawić czytelność w niektórych obszarach, może również komplikować sytuację. Osobiście opieram się na pakowaniu lub rozszerzaniu kolekcji ze względu na płynność, ponieważ nowe opakowanie, podczas pierwszego czytania, sugeruje mi, że może istnieć zachowanie, o którym muszę wiedzieć. Rozważ to odcień zasady najmniejszej niespodzianki.

Trzymanie się implementacji interfejsu oznacza, że ​​muszę się tylko martwić o interfejs. Konkretne wdrożenie może oczywiście zawierać dodatkowe zachowanie, ale nie powinienem się tym martwić. Tak więc, gdy próbuję znaleźć drogę przez czyjś kod, wolę proste interfejsy dla czytelności.

Jeśli z drugiej strony znajdujesz przypadek użycia, który korzysta z dodatkowego zachowania, masz argument za poprawieniem kodu poprzez utworzenie pełnoprawnej klasy.

Sprzężenie zwrotne
źródło
11
Opakowanie może również służyć do usuwania (lub ukrywania) niepotrzebnego zachowania.
Roman Reiner,
4
@RomanReiner - Przestrzegam przed takimi rzeczami. Niepotrzebne zachowanie dzisiaj często programista przeklina twoje imię jutro. Wszyscy wiedzą, co Listmożna zrobić i robi to wszystko z dobrego powodu.
Telastyn
Doceniam chęć utrzymania funkcjonalności, choć myślę, że rozwiązaniem jest staranne zrównoważenie funkcjonalności i abstrakcji. SoundEventCellmoże zaimplementować Iterabledla SoundEvents, który zaoferuje iterator soundEventsczłonka, więc będziesz mógł czytać (ale nie pisać) jak każdą listę. Waham się zamaskować złożoność prawie tak samo, jak waham się przed użyciem, Listgdy będę potrzebować czegoś bardziej dynamicznego w przyszłości.
Neil
2

Zawijanie go ogranicza twoją funkcjonalność tylko do tych metod, które zdecydujesz się napisać, zasadniczo zwiększając kod bez żadnych korzyści. Przynajmniej spróbowałbym:

private static class SoundEventCell : List<SoundEvent>
{
}

Nadal możesz napisać kod ze swojego przykładu.

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

To powiedziawszy, robiłem to tylko wtedy, gdy potrzebna jest funkcjonalność, której sama lista potrzebuje. Ale myślę, że twoja metoda byłaby przesadna. Chyba że masz powód, aby chcieć ograniczyć dostęp do większości metod List.

Lawtonfogle
źródło
-1

Innym rozwiązaniem może być zdefiniowanie klasy opakowania za pomocą jednej metody, która wyświetla listę:

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

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

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

Daje to dobrze znaną klasę z minimalnym kodem, ale wciąż zapewnia enkapsulację, co pozwala np. Uczynić klasę niezmienną (poprzez wykonanie kopii obronnej w konstruktorze i użycie Collections.unmodifiableListw akcesorium).

(Jednakże, jeśli te listy są rzeczywiście wykorzystywane tylko w tej klasie, myślę, że zrobisz lepiej zastąpić Map<Double, List<SoundEvent>>z Multimap<Double, SoundEvent>( docs ), jako że często oszczędza dużo zerowej sprawdzania logiki i błędów).

MikeFHay
źródło