Good or bad practice to mask Java collections with meaningful class names?

Question

Lately I've been in the habit of "masking" Java collections with human-friendly class names. Some simple examples:

// Facade class that makes code more readable and understandable.
public class WidgetCache extends Map<String, Widget> {
}

Or:

// If you saw a ArrayList<ArrayList<?>> being passed around in the code, would you
// run away screaming, or would you actually understand what it is and what
// it represents?
public class Changelist extends ArrayList<ArrayList<SomePOJO>> {
}

A colleague pointed out to me that this is bad practice, and introduces lag/latency, as well as being an OO anti-pattern. I can understand it introducing a very tiny degree of performance overhead, but can't imagine it's at all significant. So I ask: is this good or bad to do, and why?

Answer 1

Lag/Latency? I call BS on that. There should be exactly zero overhead from this practice. (Edit: It has been pointed out in the comments that this can, in fact, inhibit optimizations performed by the HotSpot VM. I don't know enough about VM implementation to confirm or deny this. I was basing my comment off of the C++ implementation of virtual functions.)

There is some code overhead. You have to create all the constructors from the base class that you want, forwarding their parameters.

I also don't see it as an anti-pattern, per se. However, I do see it as a missed opportunity. Instead of creating a class that derives the base class just for the sake of renaming, how about you instead create a class that contains the collection and offers a case-specific, improved interface? Should your widget cache really offer the full interface of a map? Or should it instead offer a specialized interface?

Furthermore, in the case of collections, the pattern simply doesn't work together with the general rule of using interfaces, not implementations - that is, in plain collection code, you would create a HashMap<String, Widget>, and then assign it to a variable of type Map<String, Widget>. Your WidgetCache cannot extend Map<String, Widget>, because that's an interface. It cannot be an interface that extends the base interface, because HashMap<String, Widget> doesn't implement that interface, and neither does any other standard collection. And while you can make it a class that extends HashMap<String, Widget>, you then have to declare the variables as WidgetCache or Map<String, Widget>, and the first loses you the flexibility to substitute a different collection (maybe some ORM's lazy loading collection), while the second kind of defeats the point of having the class.

Some of these counterpoints also apply to my proposed specialized class.

These are all points to consider. It may or may not be the right choice. In either case, your colleague's offered arguments are not valid. If he thinks it's an anti-pattern, he should name it.

Answer 2

According to IBM this actually is an anti-pattern. These 'typedef' like classes are called psuedo types.

The article explains it a lot better than I do, but I'll try to summarize it in case the link goes down:

• Any code that expects a WidgetCache cannot handle a Map<String, Widget>
• These Pseudotypes are 'viral' when using multiple packages they lead to incompatibilities while the base type (just a silly Map<...>) would've worked in all cases in all packages.
• Pseudo types are often to concrete, they do not implement specific interfaces because their base classes only implement the generic version.

In the article they propose the following trick to make life easier without using pseudo types:

public static <K,V> Map<K,V> newHashMap() {
    return new HashMap<K,V>(); 
}

Map<Socket, Future<String>> socketOwner = Util.newHashMap();

Which works due to automatic type inference.

(I came to this answer via this related stack overflow question)

Answer 3

I suggest you, as others already mentioned, to use composition over inheritance, so you can expose only methods that are really needed, with names that matches to intended use case. Do users of your class really need to know that WidgetCache is a map? And be able to do with it anything they want? Or they just need to know that is a cache for widgets?

Example class from my codebase, with solution to similar problem that you desribed:

public class Translations {

    private Map<Locale, Properties> translations = new HashMap<>();

    public void appendMessage(Locale locale, String code, String message) {
        /* code */
    }

    public void addMessages(Locale locale, Properties messages) {
        /* code */
    }

    public String getMessage(Locale locale, String code) {
        /* code */
    }

    public boolean localeExists(Locale locale) {
        /* code */
    }
}

You can see that internally it's "just a map", but public interface is not showing this. And it have "programmer-friendly" methods like appendMessage(Locale locale, String code, String message) for easier and more meaningful way of inserting new entries. And users of class can't do, for example, translations.clear(), because Translations is not extending Map.

Optionally, you can always delegate some of needed methods to internally used map.

Answer 4

The performance hit would be limited at most to a vtable lookup, which you are most likely already incurring. That's not a valid reason to oppose it.

The situation is common enough that most all statically typed programming languages have special syntax for aliasing types, usually called a typedef. Java probably didn't copy those because it originally didn't have parameterized types. Extending a class isn't ideal, due to the reasons Sebastian covered so well in his answer, but it can be a reasonable workaround for Java's limited syntax.

Typedefs have a number of advantages. They express the programmer's intent more clearly, with a better name, at a more appropriate level of abstraction. They are easier to search for debugging or refactoring purposes. Consider finding everywhere a WidgetCache is used versus finding those specific uses of a Map. They are easier to change, for example if you later find you need a LinkedHashMap instead, or even your own custom container.

Answer 5

I see this as an example of a meaningful abstraction. A good abstraction has a couple of traits:

1. It hides implementation details which are irrelevant to the code consuming it.

2. It is only as complex as it needs to be.

By extending, you're exposing the entire interface of the parent, but in many cases much of that may be better hidden, so you'd want to do what Sebastian Redl suggests and favor composition over inheritence and add an instance of the parent as a private member of your custom class. Any of the interface methods that do make sense for your abstraction can be easily delegated to (in your case) the inner collection.

As for a performance impact, it's always a good idea to optimize code for readability first, and if a performance impact is suspected, profile the code to compare the two implementations.

Answer 6

+1 to the other answers here. I'll also add that it's actually deemed to be very good practice by the Domain Driven Design (DDD) community. They advocate that your domain and the interactions with it should have semantic domain meaning as opposed to the underlying data structure. A Map<String, Widget> could be a cache, but it could also be something else, what you've correctly done In My Not So Humble Opinion (IMNSHO) is to model what the collection represents, in this case a cache.

I'll add an important edit in that the domain class wrapper around the underlying data structure should probably also have other member variables or functions which truly make it a domain class with interactions as opposed to just a data structure (if only Java had Value Types, we'll get them in Java 10 - promise!)

It will be interesting to see what impact Java 8's streams will have on all of this, I can imagine that perhaps some public interfaces will prefer to deal with a Stream of (insert common Java primitive or String) as opposed to a Java object.