Java Design Patterns – Should a Standard Factory Always Generate a New Instance?

design-patternsfactory-methodjava

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?

Best Answer

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?"

Related Topic