Is it overkill to wrap a collection in a simple class only for the sake of better readability?

Question

I have the following map:

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

This HashMap maps double values (which are points in time) to the corresponding SoundEvent 'cell': each 'cell' can contain a number of SoundEvents. That's why it's implemented as a List<SoundEvent>, because that's exactly what it is.

For the sake of better readability of the code, I thought about implementing a very simple static inner class like so:

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
}

And than the map declaration (and a lot of other code) would look better, for example:

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

Is this overkill? Would you do this in your projects?

score 14 · Answer 1 · answered Sep 17 '14 at 15:07

While it may aid in readability in some areas, it also can complicates things. I personally lean away from wrapping or extending collections for the sake of fluency, as the new wrapper, on initial reading, implies to me that there may be behavior I need to be aware of. Consider it a shade of Principle of Least Surprise.

Sticking with the interface implementation means I only need to worry about the interface. The concrete implementation may, of course, house additional behavior, but I shouldn't need to worry about it. So, when I'm trying to find my way through someone's code, I prefer the plain interfaces for readability.

If, on the other hand, you're finding a use case that does benefit from added behavior, then you have an argument for improving the code by creating a full fledged class.

score 12 · Accepted Answer · answered Sep 17 '14 at 21:28

It's not overkill at all. Start with the operations you need, rather than starting with "I can use a HashMap". Sometimes a HashMap is just what you need.
In your case I suspect it isn't. What you probably want to do is something like this:

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

You definitely don't want to have a bunch of code saying this:

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

Or maybe you could just use one of the Guava Multimap implementations.

score 2 · Answer 3 · answered Sep 17 '14 at 20:15

Wrapping it limits your functionality to only those methods you decide to decide to write, basically increasing your code for no benefit. At the very least, I would try the following:

private static class SoundEventCell : List<SoundEvent>
{
}

You can still write the code from your example.

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

That said, I've only ever done this when there is some functionality the list itself needs. But I think your method would be overkill to this. Unless you had a reason to want to limit access to most of List's methods.

MikeFHay · Answer 4 · 2014-09-17T21:22:21.563

Another solution might be to define your wrapper class with a single method which exposes the list:

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

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

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

This gives you your well-named class with minimal code, but still gives you encapsulation, allowing you to e.g. make the class immutable (by doing a defensive copy in the constructor and using Collections.unmodifiableList in the accessor).

(However, if these lists are indeed only being used in this class, I think you'd do better to replace your Map<Double, List<SoundEvent>> with a Multimap<Double, SoundEvent> (docs), as that often saves a lot of null-checking logic and errors.)

Is it overkill to wrap a collection in a simple class only for the sake of better readability?

4 Answers4