Излишне ли оборачивать коллекцию простым классом только для лучшей читабельности?

15

У меня есть следующая карта:

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

Это HashMapотображает doubleзначения (которые являются точками во времени) в соответствующую SoundEvent«ячейку»: каждая «ячейка» может содержать число SoundEvents. Вот почему он реализован как List<SoundEvent>, потому что это именно то, что он есть.

Ради лучшей читабельности кода я подумал о реализации очень простого статического внутреннего класса, например:

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
}

И чем объявление карты (и много другого кода) будет выглядеть лучше, например:

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

Это перебор? Вы бы сделали это в своих проектах?

Авив Кон
источник
Можно утверждать, что концептуально, это было решено в разделе Как вы узнали бы, написали ли вы читаемый и легко поддерживаемый код? Если ваши сверстники продолжают жаловаться на то, как вы делаете что-то, будь то так или иначе, вам лучше изменить их, чтобы они чувствовали себя лучше
комнат
1
Что делает список звуковых событий «ячейкой», а не списком? Означает ли этот выбор слов, что ячейка имеет или в конечном итоге будет вести себя иначе, чем список?
Секретный код
@DocBrown Почему? Этот класс объясняется private staticтем, что он будет использоваться только внешним классом, но он не связан с каким-либо конкретным экземпляром внешнего класса. Разве это не правильное использование private static?
Авив Кон
2
@Doc Brown, Aviv Cohn: нет тега, определяющего какой-либо язык, поэтому все может быть правильным и неправильным одновременно!
Эмилио Гаравалья
@EmilioGaravaglia: Java (я думаю, это довольно ясно, поскольку, судя по синтаксису, это может быть либо Java, либо C #, а используемые соглашения сужают его до Java;)).
Авив Кон

Ответы:

12

Это не излишне. Начните с необходимых операций, а не с «Я могу использовать HashMap». Иногда HashMap - это то, что вам нужно.
В твоем случае я подозреваю, что это не так. Что вы, вероятно, хотите сделать, это что-то вроде этого:

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

Вы определенно не хотите иметь кучу кода, говорящего это:

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

Или, может быть, вы могли бы просто использовать одну из реализаций Guava Multimap .

Кевин Клайн
источник
Таким образом, вы выступаете за использование класса, который по сути является механизмом скрытия информации, в качестве ... механизма скрытия информации? Ужас.
Роберт Харви
1
На самом деле, да, у меня есть TimeLineкласс именно для такого рода вещей :) Это тонкая оболочка вокруг HashMap<Double, SoundEventCell>(в конце концов, я пошел с идеей SoundEventCellвместо List<SoundEvent>). Так что я могу просто сделать timeline.addEvent(4.5, new SoundEvent(..))и сделать инкапсулированный материал более низкого уровня :)
Aviv Cohn
14

Хотя это может помочь в удобочитаемости в некоторых областях, это также может усложнить ситуацию. Я лично отказываюсь от обертывания или расширения коллекций ради беглости, так как новая обертка, при первоначальном чтении, подразумевает для меня, что может быть поведение, о котором мне нужно знать. Считайте это оттенком принципа наименьшего сюрприза.

Придерживаться реализации интерфейса означает, что мне нужно беспокоиться только об интерфейсе. Конкретная реализация может, конечно, содержать дополнительное поведение, но мне не нужно беспокоиться об этом. Поэтому, когда я пытаюсь найти свой путь через чей-то код, я предпочитаю простые интерфейсы для удобства чтения.

Если, с другой стороны, вы находите вариант использования, который выигрывает от дополнительного поведения, то у вас есть аргумент для улучшения кода путем создания полноценного класса.

Обратная связь
источник
11
Оболочка также может использоваться для удаления (или скрытия) ненужного поведения.
Роман Рейнер
4
@RomanReiner - я бы предостерег от подобных вещей. Ненужное поведение сегодня - часто программист проклинает ваше имя завтра. Все знают, что Listможно сделать, и делает все это по уважительной причине.
Теластин
Я ценю желание сохранить функциональность, хотя считаю, что решение - это тщательный баланс между функциональностью и абстракцией. SoundEventCellможно реализовать Iterableдля SoundEvents, который будет предлагать итератор soundEventsчлена, так что вы сможете читать (но не писать) как любой список. Я не решаюсь замаскировать сложность почти так же сильно, как не решаюсь использовать a, Listкогда мне может понадобиться что-то более динамичное в будущем.
Нил
2

Упаковка ограничивает вашу функциональность только теми методами, которые вы решили написать, в основном увеличивая свой код без пользы. По крайней мере, я бы попробовал следующее:

private static class SoundEventCell : List<SoundEvent>
{
}

Вы все еще можете написать код из вашего примера.

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

Тем не менее, я когда-либо делал это, только когда есть некоторая функциональность, которая нужна самому списку. Но я думаю, что ваш метод был бы излишним для этого. Если у вас не было причины хотеть ограничить доступ к большинству методов List.

Lawtonfogle
источник
-1

Другое решение может состоять в том, чтобы определить ваш класс-оболочку с помощью одного метода, который предоставляет список:

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

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

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

Это дает вам хорошо названный класс с минимальным кодом, но все же дает инкапсуляцию, что позволяет, например, сделать класс неизменным (делая защитную копию в конструкторе и используя Collections.unmodifiableListв методе доступа).

(Однако, если эти списки действительно используются только в этом классе, я думаю, вам лучше заменить их Map<Double, List<SoundEvent>>на Multimap<Double, SoundEvent>( docs ), поскольку это часто экономит много логики проверки ошибок и ошибок.)

MikeFHay
источник