Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
9  
It's much simpler than this. It's bad practice because I imagine your are extending the implementations of those Java basic JDK Collection. In Java, you can only extend one class, so you have to think and design more when you have an extension. In Java, use extend sparingly. –  InstructedA Jun 27 at 19:07
    
ChangeList compilation will break at extends, because List is an interface, requires implements. @randomA what you're imagining misses the point because of this error –  gnat Jun 27 at 20:38
    
Thanks @gnat +1 that was obviously a typo - fixed –  herpylderp Jun 28 at 1:02
    
@gnat It is not missing the point, I am assuming that he was extending an implementation ie HashMap or TreeMap and what he had there was a typo. –  InstructedA Jun 28 at 1:31
    
This is a BAD practice. BAD BAD BAD. Don't do this. Everyone knows what a Map<String, Widget> is. But a WidgetCache? Now I need to open WidgetCache.java, I need to remember that WidgetCache is just a map. I have to check every time I new version comes out that you haven't added something new to WidgetCache. God no, never do this. –  Miles Rout Jul 2 at 3:20

6 Answers 6

up vote 63 down vote accepted

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.

share|improve this answer
    
"There should be exactly zero overhead from this practice" - so the deopt code in HotSpot is clever enough to handle this correctly? I was pretty sure it wouldn't - any link to support that claim? I guess the cost of a polymorphic inline cache isn't that big, but it's certainly bigger than zero. –  Voo Jun 27 at 22:08
1  
This really good answer tells everyone "don't judge the decision by performance overhead" - and the only discussion here below is "how does HotSpot handle this, is there some (in 99.9% of all cases) irrelevant performance impact" - guys, did you even bother to read more than the first sentence? –  Doc Brown Jun 28 at 7:27
9  
+1 for "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?" –  user11153 Jun 28 at 15:03
5  
Practical example illustrating the main point in this answer: The documentation for public class Properties extends Hashtable<Object,Object> in java.util says "Because Properties inherits from Hashtable, the put and putAll methods can be applied to a Properties object. Their use is strongly discouraged as they allow the caller to insert entries whose keys or values are not Strings.". Composition would have been much cleaner. –  Patricia Shanahan Jun 29 at 13:49
3  
@DocBrown No, this answer claims that there's no performance impact at all. If you make strong statements you better be able to support them, otherwise people will probably call you out on it. The whole point of comments (on answers) is to point out inaccurate facts or assumptions or add useful notes, but certainly not to congratulate people, that's what the voting system is for. –  Voo Jun 29 at 15:37

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)

share|improve this answer
8  
Isn't the need for newHashMap solved now by the diamond operator? –  svick Jun 28 at 14:38
    
Absolutely true, forgot about that. I don't normally work in Java. –  Roy T. Jun 28 at 14:40

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.

share|improve this answer
    
There's also a miniscule overhead in the constructors. –  Stephen C Jun 28 at 6:45

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.

share|improve this answer

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.

share|improve this answer

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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