-1

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?

  • 1
    What is producing your "U" "R" "B" characters? Look there. I find factories that are wrappers around a switch() on a string are usually a sign if cargo cult and trouble. – whatsisname Sep 24 '17 at 1:56
  • 5
    Possible duplicate of Does this factory method pattern example violate open-close? – Doc Brown Sep 24 '17 at 6:55
  • @DocBrown read the query. how can we avoid code smell using reflection? – user1787812 Sep 24 '17 at 14:49
  • There is no indication here that you need a factory at all. – Frank Hileman Sep 24 '17 at 17:22
  • @FrankHileman Please help me understand, why this is wrong example to use factory? – user1787812 Sep 24 '17 at 17:45
2

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.

  • 1
    I always interpret "code smell" as snippets of implementation that might point to bigger problem. I know some people think all code smells are bad. No, sometimes it is a delicious cheese! – Ewan Sep 24 '17 at 18:34
  • @Ewan Irrespective of the situation, Isn't if-else-ifalways a code smell for the code written following OOP paradigm. – user1787812 Sep 25 '17 at 18:07
0

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.
0

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)

  • The concept of Repository is probably unrelated for this type of applications. Repositories are code smells in and of themselves when they introduce dependencies on global variables. – Frank Hileman Sep 24 '17 at 17:21
  • I don't understand how you got either of those impressions. That repositories arent used in games? or that repositories depend on global variables? – Ewan Sep 24 '17 at 17:57
  • Repositories typically have one or more roots in static fields that are public exposed via methods or properties. These are equivalent to global variables in that they are globally available, and the objects inside are typically mutable. I don't see why anyone would need a repository in a game. – Frank Hileman Sep 25 '17 at 17:19
  • I think you are doing repositories wrong! You should instanciate in your main class or di container like any other service. And games need repositories to encapsulate their data, levels, characters, save games everything. – Ewan Sep 25 '17 at 17:30
  • @Ewan notvsure, how to understand this comment – user1787812 Sep 26 '17 at 0:16
0

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.

  • 1
    For your point about passing "U", this answer refers an example that has similar example – user1787812 Sep 24 '17 at 18:06
  • Yes, that right but I think you get me wrong (or I did not express myself clearly). I meant that in your example you have stiff one-to-one binding. What is more I would say that example from java.nio is a bit different and the reason why it's implemented that way is that they perform some initialization for Charset classes and they want to provide consistent API for Charset types. Coming back to your example: solution with factory is better then using initialization with the new keyword but still your classes define their dependancies - it is fragile. – MPrzy Sep 24 '17 at 18:35
  • I'm using exactly this kind of code in real applications. The user makes a selection and based on it the factory creates whatever payment provider needs to be. And it's fine. In the past years I probably added a new implementation or eliminated another. Doing a factory class how you suggest it would have been a waste of resources. – Adrian Iftode Sep 24 '17 at 19:59
  • I pointed out in my post that what you really do is mapping ;) Declaring class as a factory indicates its usage as a producer of some "resource" - in your case it's Payment. If you produce payment only on user input and nowhere else in your code and you just use user input to find out which class to instantiate you just do mapping. Declaring a factory indicates that it is the proper way (or one of them) for creating particular resource. For me using factory in your code which forces you to indicate what EXACTLY needs to be created it's not the best idea – MPrzy Sep 24 '17 at 20:11
  • It's a factory because it is an object that has the knowledge of creating another object. There is no mapping there. Mapping is this kind of code: factory.Register("U", typeof(UFOEnemyShip)) – Adrian Iftode Sep 24 '17 at 20:53

Not the answer you're looking for? Browse other questions tagged or ask your own question.