Can factory pattern avoid code smell?

Question

With two existing implementations, UFOEnemyShip & RocketEnemyShip,

To introduce every new implementation implementing EnemyShip interface, shown below,

changes are needed in factory class EnemyShipFactory, with an extra else if, shown below,

public class EnemyShipFactory {
    public EnemyShip makeEnemyShip(String shipType) {
        EnemyShip theEnemy = null;

        if(shipType.equals("U")) {
            theEnemy = new UFOEnemyShip();
        }else if (shipType.equals("R")) {
            theEnemy = new RocketEnemyShip();
        }else if (shipType.equals("B")) {
            theEnemy = new BigUFOEnemyShip();
        }

        return theEnemy;
    }
}

---

CLIENT CODE:

public static void main(String[] args) {
        EnemyShipFactory shipFactory = new EnemyShipFactory();
        EnemyShip theEnemy = shipFactory.makeEnemyShip("R");
        doSomething(theEnemy);
    }

---

Is this code change in EnemyShipFactory considered a code smell? If yes, can we avoid this code smell?

Answer 1

People often focus on code smells in implementations. However, I think we also need to look at smells in our abstractions — these are often better seen by illustrating consumption/usage rather than focusing solely on implementation.

You haven't shown usage in context.

@Ewan is relating that this kind of code is seen in persistence, specifically in deserializing. In such case, the focus of the abstraction should be on pairing both serializing and deserializing together so that only one piece of code (as @Ewan suggests, your data repository) should have to know the mapping between string and subclass. This so we don't have multiple separated pieces of code somewhere each using their own copy of the string "U", for example.

However, it is also possible that you are doing something else, such as calling ... makeShip("U");. If that is the case, and you are trying to avoid directly calling new UFO(), then you might perhaps make a separate factory method dedicated to making UFO types so you don't have to pass the "U" in one place and later test for "U" in another.

Answer 2

There are a number of things you can do to make this a little bit better:

• Use a switch statement rather than chained else-if clauses
• Or possibly even better, use a dictionary of shipType to generator function.
• Change shipType from a string to an enum.

But they're all minor tweaks, and you're right that this feels like a code smell. It's impossible to give any more advice without seeing the wider context, but I'd suspect one of two things:

1. The factory pattern has been forced in here where it's not appropriate.
2. The factory needs more context to make its decision.

Answer 3

Yes, for me its a code smell. A big if block mapping magic strings to hard coded classes.

Whenever you see something like this its usually because you are generating objects from some form of persisted data, a database or a file somewhere has shiptype:"U", speed : 12, armour:6 and you want to create an instance of the appropriate class.

However, this mapping should be encapsulated in your data repository. Its part of the encoding of your data to that specific format. You shouldn't see these factory classes floating around your app.

Furthermore, with in the repository class you should avoid the magic strings by naming your tables or type strings the same as the class name.

You can also avoid the if statement with a Map of known class names to a functions which populate a new instance of the correct class with the data. ( essentially the same as a factory I know, but neater)

Answer 4

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.