0

According to Factory Method pattern, we delegate object creation to a separate class. Let's say I have abstract class A and some implementors. Implementation is loaded from configuration file with reflection and thus we still operate with type A. I wonder if there is anything wrong with creating static creation method in abstract class. Any comments? The benefit of it is that no additional class is needed. You could argue that besides main functionality it also has the responsibility of creating subtyped objects - would that really violate S principle from SOLID?

public abstract class A
{
 // Some abstract stuff

 public static A CreateInstance()
 {
   Type type = GetTypeFromConfig(); // pseudo method
   return (A)Activator.CreateInstance(type);  
 }
}
Gilad Green
  • 36,708
  • 7
  • 61
  • 95
Nickolodeon
  • 2,848
  • 3
  • 23
  • 38
  • So you would like to call a Create method defined in class A on the objects of derived classes, right? – Zegar Aug 09 '16 at 09:25
  • Please see sample code edit. So because it is static I would call this method on type A. – Nickolodeon Aug 09 '16 at 09:34
  • @Nickolodeon - thanks for adding the code. Makes it more concrete. In the case above how would you use it in a place where you need an instance of a derived from `A` class? – Gilad Green Aug 09 '16 at 09:36
  • Implement A with MyNewDerivedClass and put MyNewDerivedClass' type to configuration file from where that CreateInstance() is reading it from. So no modification is needed to this 'factory method'. – Nickolodeon Aug 09 '16 at 09:41

2 Answers2

1

I'm not sure your implementation falls into Factory Method :

Define an interface for creating an object, but let subclasses decide which class to instantiate. The Factory method lets a class defer instantiation it uses to subclasses.

(https://en.wikipedia.org/wiki/Factory_method_pattern)

Here, your CreateInstance() method isn't derived in concrete classes, it gets a single implementation right at the top of the class hierarchy.

Furthermore, it has one major drawback - the method is static. It basically means that every unit test that wants to freeze generated objects to an arbitrary subtype of A will have to specify that subtype in their config file. And the subtype will be the same across the whole test project.

With a nonstatic method (which only makes sense in a separate dedicated Factory class), you could juggle different implementations of that object generator much more easily - using stubs for instance.

guillaume31
  • 13,738
  • 1
  • 32
  • 51
  • After looking at factory method more attentively I see that pattern we discuss (the one posted initially) does not fall into Factory Method pattern. Indeed, there must be abstract method for each derived type to define how exactly they are creating themselves. And again, question arises - why not just use constructors? I feel all points by guillaume31 are valid. – Nickolodeon Aug 09 '16 at 11:43
  • Your solution still allows changing strategy without editing the source code, which can be useful. But a static method... I wouldn't do it that way. – guillaume31 Aug 09 '16 at 12:04
  • It's the simplest solution though - and that's the advantage. What's wrong with static and how would you implement it otherwise? Note that config information is always sufficient (by design) for Activator. – Nickolodeon Aug 09 '16 at 14:05
  • Depends what you need it for. You should post a sample of code that uses `CreateInstance()`, with a bit of context. But basically, I would have an abstract class or interface for the Factory and an implementation of it that uses the config file. In unit tests, I would stub the factory to return whatever I need it to generate in that test. – guillaume31 Aug 09 '16 at 14:51
0

EDIT: Yeah now with the code it's more clear.

SRP is validated with another function of A - that's for sure.

What's more I would be concerned with all the cases when someone tries to use your factory to create instance of class defined in config file but not derived from A. That would result in cast exception of course.

I can easily imagine that in a relatively big project after a few weeks or months someone is going to edit this config file and cause a cast exception there.

You're kind of getting rid of static type control by doing this.

Zegar
  • 1,005
  • 10
  • 18
  • I'm sorry for not grasping last phrase "You're kind of getting rid of static type control by doing this.". Can you clarify what you mean? – Nickolodeon Aug 09 '16 at 09:55
  • Sure, normally when you're using a factory pattern you can create objects of derived classes so you are SURE in the compile-time that every instance coming out of `A` factory is indeed an instance of `A`. In your example you overcome this requirement. Depending on what types are defined in your config you are going to make your `A` factory to create objects of that type. There is no way to know in the compile time if it's going to work or throw a cast exception. – Zegar Aug 09 '16 at 10:00
  • Yes, cast exceptions are possible. Going to add extra check that type indeed derives from A (I guess via type.IsInstanceOfType()). – Nickolodeon Aug 09 '16 at 11:45