CODEWITHSUNDEEP
javadesign-patterns
Dev Blanked
Dev Blanked

Reputation: 8885

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

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

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

Upvotes: 3

Views: 1144

Answers (4)

Dev Blanked
Dev Blanked

Reputation: 8885

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

Upvotes: 0

Tom Hawtin - tackline
Tom Hawtin - tackline

Reputation: 147164

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.

Upvotes: 2

Stephen C
Stephen C

Reputation: 719299

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.

Upvotes: 4

Evgeniy Dorofeev
Evgeniy Dorofeev

Reputation: 136062

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

Upvotes: 2

Related Questions