AbstractClass.getInstance() method is this an anti-pattern

Question

In some places where a class hierarchy is present and the top most base class is an abstract class there is a static getInstance() method in the abstract class. This will be responsible for creating the correct sub-class and returning it to the caller. For example consider the below code.

public class abstract Product {

   public static Product getInstance(String aCode) {
       if ("a".equals(aCode) {
          return new ProductA();
       }
       return ProductDefault();
   }
   // product behaviour methods
}

public class ProductA extends Product {}

public class ProductDefault extends Product {}

In Java, java.util.Calendar.getInstance() is one place this pattern has been followed. However this means each time a new subclass is introduced one has to modify the base class. i.e: Product class has to be modified in the above example. This seems to violate the ocp principle. Also the base class is aware about the sub class details which is again questionable.

My question is...

is the above pattern an anti-pattern ?
what are the draw-backs of using the above pattern ?
what alternatives can be followed instead ?

Quite the opposite. It's one of the most famous patterns, the Factory Method. — Thilo, May 04 '13 at 05:52
It does not have to implemented with hard-coded if statements. Registries are quite common, where the individual subclasses tell the factory about their existence. — Thilo, May 04 '13 at 05:55
@SpencerRuport: That sounds like the singleton pattern (which java.util.Calendar certainly is not) — Thilo, May 04 '13 at 05:56
As many others have said, this is a factory method. However whilst thinking about alternatives, you should prefer composition over inheritance. See http://stackoverflow.com/questions/49002/prefer-composition-over-inheritance — crowne, May 04 '13 at 06:01
@Thilo isn't factory method about having a separate factory class rather than having the getInstance() method inside the base class — Dev Blanked, May 04 '13 at 06:43
@DevBlanked: That it's called factory *method* seems to imply that you don't need a separate factory class. FWIW, in the JDK, getInstance on abstract base class is very common. — Thilo, May 04 '13 at 06:46

Stephen C · Answer 1 · 2013-05-04T07:10:30.770

The interface is not an anti-pattern. But the way you've implemented it is rather poor ... for the reason you identified. A better idea would be to have some mechanism for registering factory objects for each code:

The Java class libraries do this kind of thing using SPIs and code that looks reflectively for "provider" classes to be dynamically loaded.
A simpler approach is to have a "registry" object, and populate it using dependency injection, or static initializers in the factory object classes, or a startup method that reads class names from a properties file, etcetera.

score 2 · Answer 2 · answered May 04 '13 at 05:54

2

No it's not. It's more like factory method pattern http://en.wikipedia.org/wiki/Factory_method_pattern. E.g. Calendar.getInstance();. JDK is full of such examples. Also reminds of Effective Java Item 1: Consider static factory methods instead of constructors

answered May 04 '13 at 05:54

Evgeniy Dorofeev

133,369
30
199
275

I think effective java talks about a single product class rather than a hierarchy – Dev Blanked May 04 '13 at 06:40
"A third advantage of static factory methods is that, unlike constructors, they can return an object of any subtype of their return type." – Evgeniy Dorofeev May 04 '13 at 06:45
1

If you were to design `Calender` again, you would do it differently (Actually Java 8 will) I would look at a more recent class like `Logger.getLogger(String name);` – Peter Lawrey May 04 '13 at 07:08

score 2 · Answer 3 · answered May 04 '13 at 07:41

There are a number of separate issues here.

getInstance is probably going to be a bad name. You explicitly want a new object you can play around with. "Create", "make", "new" or just leave that word out. "Instance" is also a pretty vacuous word in this context. If there is sufficient context from the class name leave it out, otherwise say what it is even if that is just a type name. If the method returns an immutable object, of is the convention (valueOf in olden times).

Putting it in an abstract base class (or in an interface if that were possible) is, as identified, not the best idea. In some cases an enumeration of all possible subtypes is appropriate - an enum obviously and really not that bad if you are going to use visitors anyway. Better to put it in a new file.

Anything to do with mutable statics is wrong. Whether it is reusing the same mutable instance, registration or doing something disgusting with the current thread. Don't do it or depend (direct or indirectly) on anything that does.

score 0 · Answer 4 · answered May 11 '13 at 07:51

Based on the feedback i introduced a new ProductFactory class that took care of creating the correct Product. In my case the creation of the correct product instance depends on an external context (i've put the product code for the purpose of simplicity.. in the actual case it might be based on several parameters.. these could change over time). So having a Product.getInstance() method is not that suited because of the reasons outlined in the question. Also having a different ProductFactory means in the future.. Product class can become an interface if required. It just gives more extensibility.

I think when the creation of the object doesn't depend on an external context.. like in the case of Calendar.getInstance() it's perfectly ok to have such a method. In these situations the logic of finding the correct instance is internal to that particular module/class and doesn't depend on any externally provided information..

AbstractClass.getInstance() method is this an anti-pattern

4 Answers4