16

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?

5
  • one may argue that conceptually, this has been addressed in How would you know if you've written readable and easily maintainable code? If your peers keep complaining about your way of doing things, be it one way or another, you better change to make them feel better
    – gnat
    Sep 17 '14 at 14:44
  • 1
    What is it that makes the list of sound events a "cell" rather than a list? Does this choice of words mean that a cell has or eventually will have different behavior than a list?
    – x-code
    Sep 17 '14 at 15:45
  • @DocBrown Why? The class is private static because it's only going to be used by the outer class, but it isn't related to any specific instance of the outer class. Isn't that exactly the proper usage of private static?
    – Aviv Cohn
    Sep 17 '14 at 20:12
  • 2
    @Doc Brown, Aviv Cohn: There is no tag specifying any language, so anything can be right and wrong at the same time! Sep 17 '14 at 20:24
  • @EmilioGaravaglia : Java (I think it's pretty clear since judging by the syntax it could be either Java or C#, and the conventions used narrow it down to Java ;) ).
    – Aviv Cohn
    Sep 17 '14 at 20:47
12

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.

2
  • So you advocate using a class, which is essentially an information-hiding mechanism, as an... information hiding mechanism? The horror. Sep 17 '14 at 21:29
  • 1
    Actually yeah, I do have a TimeLine class exactly for that kind of thing :) It's a thin wrapper around a HashMap<Double, SoundEventCell> (eventually I did go with the SoundEventCell instead of List<SoundEvent> idea). So I can just do timeline.addEvent(4.5, new SoundEvent(..)) and have the lower-level stuff encapsulated :)
    – Aviv Cohn
    Sep 19 '14 at 11:35
14

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.

3
  • 11
    A wrapper can also used to remove (or hide) unneeded behavior. Sep 17 '14 at 20:17
  • 4
    @RomanReiner - I'd caution against such things. Unneeded behavior today is often the programmer cursing your name tomorrow. Everyone knows what a List can do, and it does all of those things for a good reason.
    – Telastyn
    Sep 17 '14 at 20:37
  • I appreciate the desire to maintain functionality, though I think the solution is a careful balance between functionality and abstraction. SoundEventCell could implement Iterable for SoundEvents, which would offer the iterator of soundEvents member, so you would be able to read (but not write) as any list. I hesitate to mask complexity almost as much as I hesitate to use a List when I may need something more dynamic in the future.
    – Neil
    Sep 19 '14 at 7:41
2

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.

-1

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.)

Not the answer you're looking for? Browse other questions tagged or ask your own question.