3

According to my understanding on factory-method, factory should always return a new instance, meaning no cache, So in essence, every time when the factory method is called, there should always be a new instance returned, otherwise, it would not be a factory method pattern. For instance, the following code is not a factory:

public class UserManagementActionListenerFactory extends AbstractListenerFactory {
    private static final String ENABLE_USER = "enableUser";
    private static final String DISABLE_USER = "disableUser";
    private static final String DELETE_USER = "deleteUser";
    private static final String CONVERT_USER = "convertUser";
    private Map<String,ActionListener> mapper;
    public ActionListenerMapping() {
        mapper = registerListeners();
    }
    private Map<String, ActionListener> registerListeners() {
        Map<String,ActionListener> actionListenerMap = new HashMap<>();
        actionListenerMap.put(ENABLE_USER, new EnableActionListener());
        actionListenerMap.put(DISABLE_USER, new DisableActionListener());
        actionListenerMap.put(DELETE_USER, new DeleteActionListener());
        actionListenerMap.put(CONVERT_USER, new ConvertUserActionListener());
        return actionListenerMap;
    }
    @Override
    ActionListener getActionListener(String s) {
        return mapper.get(s);
    }
}

The UserManagementActionListenerFactory is used in a bean called UserManagementBean:

public class UserManagementBean {
    AbstractActionListenerFactory factory;
    @PostConstruct
    public init() {
        factory = new UserManagementActionListenerFactory();
    }
    //... irelevant code
    public void handleActionEvent(ActionEvent e) {
        String componentId = e.getComponent().getId();
        factory.getActionListener(componentId).processAction(e);
    }
}

As this ActionListenerMapper initialized all the concrete subclasses of ActionListener when being constructed, then every time the getActionListener(s) is called, it always returns a cached ActionListener instance. Please tell here if my understanding is wrong

However, in first item of the book Effective Java:

A second advantage of static factory methods is that, unlike constructors, they are not required to create a new new object each time they're invoked.

Moreover, Effective Java also told:

Note that a static factory method is not the same as the Factory Method pattern from Design Patterns [Gamma95]

Question: Usually should a factory method always return a new instance instance?

1

While the other answers properly address most of the concerns regarding the usage of a factory and the factory pattern in general, I would only like to add that your class is not necessarily a factory. That is, not until you can reason properly about why it is a factory.

A factory is not a factory because the class name ends in -Factory, obviously. By all means, your question and the answers already cover issues regarding the expected behavior of a factory, but just making a class that provides access to something (cached or not cached) does not make it a factory.

Based on your code (without any more context), I cannot see why a class/interface method, which receives a string and returns action listeners, would be considered a factory. I don't see any difference between your class and an identical class named ActionListenerRepository with a getActionListenerByID(string) instead of getActionListener(string) (or even the same). Besides, callers of the method (apparently, from your code) don't really know how, when or why the returned instances were created/added.

Patterns are good, knowing how to utilize them properly even more so, but you must not skip semantics.

So, what you should clarify is: Why is this a factory to you and your object model?

Edit

After the question has been edited to include a specific use-case, and after the exchange in the comments, an update is made to the answer.

Let's try to look at it from the code reviewer's perspective. Coding is all about meaning and semantics. When we write high-level code, think of it as giving instructions to someone (i.e. not to the computer). When we refer to a factory, a factory comes to mind. A factory produces things, it does not, usually, store things. It produces them. So when a reviewer sees a factory that is, in fact, a storage place, instead, the boundary is crossed between intended and expected meaning. Mixing up the two violates the principle of least astonishment, which is "jargon" for not getting what you expected.

What the "client" expects is not all that important, because it is just as easy for the client to get something from the factory and cache it themselves. But think about it in the intuitive sense. Would you request something from a factory each time you need to use it?

In short, the primary reason your factory is not a factory is that you are neither building it as such, nor using it as such. The expected usage of a factory is to:

  • Request a thing every now and then (as in "not every time").

  • Expect something new most of the time.

  • Keep the returned object for further use.

Your class is used in a totally opposite way. It requests the factory's result at every use, expects the same object anyway, because who would like to get different action listeners upon each request, and does not keep them stored inside the client class.

Last, but not least

Because you also mentioned caching, factories are not paired with caching in the way you seem to think they are. Factories don't typically cache objects for multiple returns. Factories cache new instances for immediate returns. This means that, while the user of an application is doing other things, a factory can spawn a couple of tasks/threads that construct new instances, so that the next time a request is made, the new instance is at the ready, hence immediately returned, and, of course, (potentially forever) forgotten by the factory (unless you are doing instance counting, etc. for whatever reason).

So, caching, in the context of a factory, has a more useful meaning as having pre-constructed objects to hand out immediately, not as handing out the same objects each time. When you pass an instance reference to someone, you immediately forfeit "total" ownership. Now someone else can play with your object, so, I am asking you, as a factory: "Are these objects yours after you give them away?"

| improve this answer | |
  • But in the book Head First Design Patterns, the code example of a simple factory for public Pizza createPizza(String type) is just like the code I have given. The only difference is that in the book the createPizza(String type) always return a new pizza instance – Rui Dec 1 '19 at 12:51
  • This sounds reasonable enough. Pizza is created and it makes all the sense in the world that a method termed create will actually "create" something. And, also, when you actually go to a restaurant and order a pizza, you cannot really know if you are being served a new pizza or a heated pizza that was "created" earlier. You can simply swap the new pizza with frozen pizza that has just been reheated and the restaurant will still run. My point is, there are concise meanings and a fairly intuitive connection to the corresponding domain when it comes to pizza. – Vector Zita Dec 1 '19 at 13:00
  • ActionListener objects, however, are not so straightforward to reason about. Pizza, we know about. We know how it is used, and what for (well, usually..). But what is a factory for action listeners? What is it useful for? What are action listeners used for and how are they used. These are things you have to reason about before you can claim a factory. Also, factories usually produce objects for "consumption", which means they may be modified after you pass them out. Are your ActionListener objects for "consumption"? – Vector Zita Dec 1 '19 at 13:05
  • If I would read your class content skipping the name entirely, I would say that this resembles an event publisher. You don't need to pass the action listeners, but maybe "expose" what they do through specific methods (e.g. register or subscribe). If your intended use is to attach events, "leaking" the actual implementations may not be very safe in the long run, let alone does not actually give your class a "character". Do you intend that every other class with a reference to the factory can actually "tamper" with the action listeners as it wishes? Is this safe, as per your design? – Vector Zita Dec 1 '19 at 13:09
  • I added also the code snippet, where the UserManagementActionListenerFactory is used. So now you can :) is it a factory after all? – Rui Dec 1 '19 at 13:49
4

No, Joshua Bloch is right (as he usually is). A factory or factory method need not create a new instance for every call. That is, in fact, one of the advantages of factories.

Look at it this way: the point of a factory is to relieve a client programmer from having to understand the exact type hierarchy of a resource. This means that you can replace your sturdy StandardStructure with a snazzy ImprovedStructure and roll it out without having to change any client code. This is a good thing.

Likewise, the customer of a factory doesn't have to understand the details of the lifecycle of your objects. They may be good for one client only, or they may be smart enough to serve many masters. They may be able to deal with any number of calls, or have a resource (maybe bits of entropy) that decays with use. No matter - the factory knows how to manage object lifetimes correctly, meaning that the client programmer doesn't have to understand it. The result is the same win in maintainability as for the details of your type hierarchy.

| improve this answer | |
  • Thanks a lot for your explanation. In this case the code I placed in the question is also a factory? – Rui Nov 29 '19 at 10:45
  • I'd say it is: it yields ActionListeners and conceals the concrete subclasses that it actually creates. It doesn't cache anything, but a factory doesn't have to cache things. The point is that it can (or can some day be changed so that it does). – Kilian Foth Nov 29 '19 at 10:47
  • The code was actually reviewed by my superior as not a factory. That is why I asked :) But the ActionListener subclasses are all stored into the HashMap during construction, isn't this a cache mechanism? or might I have misunderstood the concept of cache? – Rui Nov 29 '19 at 10:50
  • You're right, a cache is a structure that enables at least one execution path where you no longer have to construct anything when asked for it, but simply return something that you constructed earlier. In that sense this is a cache, since the second call to getActionlistener("x") will return the same object as the first. – Kilian Foth Nov 29 '19 at 11:01
  • 2
    @pschill I could not disagree more. The problem with global variables is not that they occupy storage - computers have more than enough of it. The problem is that their user must keep mental account of what entities there are in the system. Since the factory takes care of this, it is a maintainability plus, not a minus. – Kilian Foth Nov 29 '19 at 11:19
2

I see two different possible interpretations of this question:

  1. should a factory method always return a new instance instance - because the term "factory method" is broadly accepted in software development to describe functions which behave like this?

So the solution would be here to give the method a different name other than one containing the term "factory"?

Or

  1. should a factory method always return a new instance instance - because if it does not, will this become prone to errors?

So the solution is not to write such methods?

The answer to the first question is definitely "no". As with many terms in software engineering, the name "factory method" has more than one valid meaning, there is no "mathematically strict definition" for it. Factories are expected to choose between different subtypes and deliver objects one otherwise had to create by new, but if they call new by themselves, or are implemented differently should not matter. You can define whatever convention you like for your factories, and if you see a valid reason why you think it is necessary in your system to make certain factories to always create new instances, then go ahead and establish this convention in your team. Or you can allow factory method to behave differently, there is no "law" which forbids this.

The answer to the second question is: it is usually safe for a factory to deliver the same instance of an immutable object more than once if the input values of the factory method are the same. I guess in your example above, every ActionListener is either immutable or treated in an immutable fashion. If the returned objects are not immutable, returning the same instance twice could lead to all kind of unwanted side effects, so it would be less error prone to deliver always a newly created instance.

There is another design where object reuse may be ok even for mutable objects: that's when your factory manages some resource pool (for example, a thread pool), and users have to allocate "resource objects" from there and return the used objects after usage explicitly. Of course, it is debatable if for such cases the name "factory" would still be a good fit.

Let me add a note about your example code: from the viewpoint of a caller of that function getActionListener, it does most probably not matter if a call to this function always produces a new object, or if the same (immutable) object is returned on two calls with the same input parameter. Hence if that code snippet is better called a "factory" or a "mapper" is mostly a matter of taste - the name should not depend on how the function is implemented internally.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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