Is it a code smell to have a static factory method on the base class?

Question

Suppose we have a BaseModel, which has a type enum, and derived models with same constructor signatures to each other, whose implementations are like :

public DerivedModelJ(Object arg1, ..., Object argN) {
  super(Types.TypeJ, arg1, ... argN)
}

Now suppose that, when we use them, we want to hit a method that will return the right model, but don't want to sort through it on that side (let alone wherever else you might end up needing it in the future).

Would it be a code smell to be like:

public static BaseModel Create(Types type, Object arg1, ..., Object argN) {
  HashMap<Types, Class> typesDict = new HashMap();
  typesDict.put(Types.Type1, DerivedModel1.class);
  //...
  typesDict.put(Types.TypeM, DerivedModelM.class);
  return typesDict.get(type).newInstance(arg1, ..., argN);
}

on BaseModel?

Answer 1

Maybe not a code smell, but...

1. Coupling: Assuming all classes are public, they will be in separate .java files. Your derived classes will need to import the base class as usual, but the base class will also need to import the derived classes. This produces a circular reference. This may compile in Java (if I remember correctly), but is not allowed in some languages, so I always avoided it as a matter of principle. I suggest you move the static method out of the base class to fix this, or perhaps consider the code suggested by @CarmineIngaldi
2. Duplication of information: The class type and your Enum seem to carry the same information. Can your Enum be discarded? If you really need to test a class type, you can always use the instanceof keyword although this often a sign that some base class method should be overridden in the derived classes.
3. Principle of least astonishment: When scanning an unfamiliar code base, I would not expect to find logic in a base class that decides which descendant is to be instantiated. This logic would normally be found in a more business-oriented higher layer, typically in a simple switch statement.

Hope this is of some help.

Answer 2

It is not a code smell per se, the goal of a factory is to hide every detail about the creation of an object, even its type where applicable. The Liskov Substitution principle tells us that every type can replace its supertype and the logic which creates instances of these types is somehow related to the supertype (recalling that, as static method, the supertype here acts as a sort of "namespace" for the declaration of the method)

there are two things that convince me less

1. I think you are using a "slow" creation (create a map, then instantiate an object using reflection APIs) only for avoiding a switch/case construct. Have you considered to use your enum directly for instantiating the object? (see snippet below)

2. what distinguishes subclasses? they have the same fields, only the value that you are assigning to "type" differs, If even the behaviour doesn't change, maybe you don't need inheritance.

   public enum Type {
   
     TYPE_1 {
       @Override
       public SomeClass create() {
         return new SubclassOfSomeClass();
       }
     }    
   
     public abstract Someclass create();
   }
   

Answer 3

The problem is that the base class knows its descending types. This violates the open-closed principle (the O in SOLID). Every time you come up with a new descending type you need to revisit the base class and change it. This defeats a pillar of object orientation.