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?

Aviv Cohn
  • 21,190
  • 31
  • 118
  • 178
  • one may argue that conceptually, this has been addressed in [How would you know if you've written readable and easily maintainable code?](http://programmers.stackexchange.com/a/141010/31260) 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! – Emilio Garavaglia 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

4 Answers4

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.

FeedbackLoop
  • 151
  • 4
  • 11
    A wrapper can also used to *remove* (or hide) unneeded behavior. – Roman Reiner 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 `SoundEvent`s, 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 07:41
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.

kevin cline
  • 33,608
  • 3
  • 71
  • 142
  • So you advocate using a class, which is essentially an information-hiding mechanism, as an... information hiding mechanism? The horror. – Robert Harvey 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` (eventually I did go with the `SoundEventCell` instead of `List` 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
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.

Lawtonfogle
  • 443
  • 3
  • 12
-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.)

MikeFHay
  • 506
  • 2
  • 8