Before I start I have to say that this example probably will never exist in that shape in real app (at least it should not).
That's why (IMHO):
1) You directly point the implementation from client code:
First of all you are passing string to makeEnemyShip method - it is really error prone practice to passing type information as a string BUT the sad thing about that example is that your input is some sort of direct indicator for implementation.
I mean that your shipType is pointing EnemyShip implementation by one to one mapping between input string and ship classes.
If you get input argument from the "outside world" (DB, API, UI etc) this might be reasonable to provide some sort of mapping or simple factory (like this from your example) which just pick proper factory method (or just instantiate a specific class) based on "outside input".
class PaymentProvider {
private creators: { [key: string]: PaymentFactoryMethod };
// ...
public getPayment(paymentSignature: string): Payment | undefined {
const createPayment = this.creators[paymentSignature];
return createPayment ? createPayment() : undefined;
}
}
BUT if you want to provide this factory for general usage,
I assume that you may end up passing it to one of your classes. Therefore you might want to do something like:
EnemyShip enemy = factory.makeEnemyShip("U")
// or what's worse:
this.enemy = this.factory.makeEnemyShip("U")
For me this code violates the one of the most important rule of factory pattern:
Let the client code to be unaware of implementation provided by factory.
However defining factory this way still has some advantages I think main is encapsulation of logic which stands behind creating enemy ship (it may be complex).
In my opinion generally it's better to pass factory as dependency and use Factory Method or Abstract Factory pattern, and what is most important when it comes to factories consider if you really need it.
2) Your code violates the open/close principle
As you've noticed this code has predefined all possible EnemyShip implementations - which is often (even very often - but not always) a bad idea.
Because:
- it's difficult to add new type - you have to refactor this code
- it's not dynamic
Fix for this hardcoded "if" statement issue is Map or Dictionary. Right now you cannot add new implementation during runtime - If you had a Dictionary (or Map) which maps types or predicates to particular classes you would easily register and deregister new factories.
HOWEVER You would still need some place to configure this factory.
To sum up
I think that example you've provided is some sort of learning material which learns how to get rid of new
keyword rather than provides real app useful pattern.