Java – Can factory pattern avoid code smell

code smellcode-qualitydesign-patternsfactory-patternjava

With two existing implementations, UFOEnemyShip & RocketEnemyShip,

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

enter image description here

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?

Best Answer

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.

Related Topic