Factory Pattern in C++ -- doing this correctly?

Question

I am relatively new to "design patterns" as they are referred to in a formal sense. I've not been a professional for very long, so I'm pretty new to this.

We've got a pure virtual interface base class. This interface class is obviously to provide the definition of what functionality its derived children are supposed to do. The current use and situation in the software dictates what type of derived child we want to use, so I recommended creating a wrapper that will communicate which type of derived child we want and return a Base pointer that points to a new derived object. This wrapper, to my understanding, is a factory.

Well, a colleague of mine created a static function in the Base class to act as the factory. This causes me trouble for two reasons. First, it seems to break the interface nature of the Base class. It feels wrong to me that the interface would itself need to have knowledge of the children derived from it.

Secondly, it causes more problems when I try to re-use the Base class across two different Qt projects. One project is where I am implementing the first (and probably only real implementation for this one class... though i want to use the same method for two other features that will have several different derived classes) derived class and the second is the actual application where my code will eventually be used. My colleague has created a derived class to act as a tester for the real application while I code my part. This means that I've got to add his headers and cpp files to my project, and that just seems wrong since I'm not even using his code for the project while I implement my part (but he will use mine when it is finished).

Am I correct in thinking that the factory really needs to be a wrapper around the Base class rather than the Base acting as the factory?

@sbi If you look at some other comments I've made in the past, I tend to agree. However, when I have a question regarding code architecture, I've got to do my best to use the nomenclature of the community in order to convey the problem. It seems there is a consensus that my thoughts are correct: doing it the current way is limiting and will cause problems. By using the "factory" terminology, I was able to convey my question. Do you see the advantage in my doing this? — San Jacinto, Feb 14 '11 at 13:14
I wasn't trying to judge what you've asked about here. I was merely making a general statement. I now see that this must have seemed like an assault against your question. Sorry for that. — sbi, Feb 14 '11 at 22:09
@sbi I'd say it was more that your comment left me asking "is it your opinion that it is wrong to try to use the common terminology for a technique, or is it that I shouldn't be using this technique?" than that I felt assaulted. Your answers and comments on SO are usually rather terse, so while I seek out your writings when they are applicable to something I'm looking up, I often wish you'd share as much reasoning as you can for your statements since I know that you personally have good reasoning. Thus my comment to your comment. — San Jacinto, Feb 15 '11 at 01:36
Thank you very much for this feedback! In the future I will try to not to be terse in my answers. — sbi, Feb 15 '11 at 06:06

score 20 · Accepted Answer · answered Feb 14 '11 at 12:56

20

You do NOT want to use your interface class as the factory class. For one, if it is a true interface class, there is no implementation. Second, if the interface class does have some implementation defined (in addition to the pure virtual functions), making a static factory method now forces the base class to be recompiled every time you add a child class implementation.

The best way to implement the factory pattern is to have your interface class separate from your factory.

A very simple (and incomplete) example is below:

class MyInterface
{
public:
    virtual void MyFunc() = 0;
};

class MyImplementation : public MyInterface
{
public:
    virtual void MyFunc() {}
};

class MyFactory
{
public:
    static MyInterface* CreateImplementation(...);
};

answered Feb 14 '11 at 12:56

Zac Howland

15,777
1
26
42

1

Exactly right – except there's no reason for CreateImplementation to be static. It should either be a non-static method or a free function. – Fred Nurk Feb 14 '11 at 13:06
Ok. Thanks. This was my thought. I'll start the move before he gets in. The less work he has to do, the more likely he is to accept the alternative :) – San Jacinto Feb 14 '11 at 13:11
2

+1 Although, I would like to see a smart_ptr or copy being returned by *CreateImplementation* and not a raw pointer. – T33C Feb 14 '11 at 13:11
What T33c says: just return a plain value rather than a pointer, and rely on the compiler doing RVO for performance - unless you have specific needs requiring otherwise, and then use a smart pointer container. – Eamon Nerbonne Feb 14 '11 at 13:28
@Fred: Correct. It could be a free function. I left it in its own class to show the point that you do not want it in your interface definition. – Zac Howland Feb 14 '11 at 13:43
4

@Eamon: no, you can't return an abstract base class by value; the factory has to return a pointer, smart or otherwise. – Mike Seymour Feb 14 '11 at 13:52
In general `CreateImplementation` can't return by value. The runtime type of an object created by a factory function might depend on the arguments to the factory function (or if the factory function is a member function, also on dependencies injected into the object it's called on). That's the whole point of calling a factory function rather than a constructor - you don't care what class is returned. Returning a smart pointer is OK (within a DLL) only if either: everyone uses the same smart ptr for all purposes; or you choose a smart pointer with a `release()` function. Beware `shared_ptr`. – Steve Jessop Feb 14 '11 at 13:57
Devils advocate: why does it matter that the base class must be recompiled every time a class is added? How is this worse than requiring the Factory class to be recompiled every time a class is added? After all, the class interface isn't changing, so this won't cause cascading other updates and lots of compile-time slowdown. – Eamon Nerbonne Feb 14 '11 at 14:03
@Eamon: Why do we put things in classes at all? Why not make every application a single massive monolithic entity. It is not just compilation, it also requires every child class to be relinked, and logically, just puts a method in a place it doesn't below. Having worked on a project where the previous engineer had done this exact bad practice, it is far better to have the factory method outside the base class. – Zac Howland Feb 14 '11 at 14:14
5

@Eamon: as well as what Zac says, in a design that isn't rubbish it's likely that more things use the interface than use the factory, since some things will take objects as parameters and act on them, but won't create the objects themselves. So you can reduce coupling/dependencies by ensuring that things that only need the interface don't also depend on the factory function. Fundamentally the idea of "the" factory function is flawed. If the interface is a worthwhile abstraction, there's probably more than one conceivable way to create an object implementing it. – Steve Jessop Feb 14 '11 at 15:29
Thanks for the feedback! To be clear: I'm brainstorming and could be spouting nonsense (e.g. the return-by-value nonsense :-) ). Including the (static) factory method in the interface would not require child classes to be relinked: effectively, the interface is no more than an easily-remembered namespace, right? So isn't this just a tradeoff between simplicity in the sense of avoiding large APIs and fine-grained control? E.g. many interfaces could be split into components, but at some point the extra precision isn't worth the complexity. – Eamon Nerbonne Feb 17 '11 at 10:07
@Eamon: You aren't increasing the size of the API; you are organizing the API such that each interface/implementation serves 1 purpose. When you start having objects serve multiple purposes, you increase the complexity of your code (and usually end up having to make special cases for certain states). – Zac Howland Feb 17 '11 at 12:28
But there's no object involved - it's a static method, after all. The factory method might well be in a seperate compilation unit from other code concerning the interface - it's just namespacing. And it seems natural enough to avoid namespace pollution by having the interface *be* the namespace for the factory method for that interface. – Eamon Nerbonne Feb 18 '11 at 13:45
@Eamon: This comes from experience - it is not natural to do that. Group your code logically and other programmers will thank you for it. Try to group it "out of convenience", and you will have your picture on the wall with darts thrown at it regularly. – Zac Howland Feb 18 '11 at 13:57
1

So the argument is that it's conventional to have all implementation details concerning an abstract base class in one file; and if so, having a factory method on the ABC means recompiling the ABC whenever a subclass is added (though that's probably cheap if your ABC has a small implementation). Additionally, Steve suggests making the factory a separate API to minimize the API surface. On the other hand, an extra code file and extra namespace have their own intrinsic costs. Which suggests to me that the more widely used the interface is, the more worthwhile it is to separate the factory out. – Eamon Nerbonne Feb 18 '11 at 14:09

score 4 · Answer 2 · answered Feb 14 '11 at 13:05

4

I'd have to agree with you. Probably one of the most important principles of object oriented programming is to have a single responsibility for the scope of a piece of code (whether it's a method, class or namespace). In your case, your base class serves the purpose of defining an interface. Adding a factory method to that class, violates that principle, opening the door to a world of shi... trouble.

answered Feb 14 '11 at 13:05

Victor Welling

1,867
12
14

Very true indeed. A base class is a just a base class, not a factory for itself or descendants. – GolezTrol Feb 14 '11 at 13:18

score 3 · Answer 3 · answered Feb 14 '11 at 12:57

Yes, a static factory method in the interface (base class) requires it to have knowledge of all possible instantiations. That way, you don't get any of the flexibility the Factory Method pattern is intended to bring.

The Factory should be an independent piece of code, used by client code to create instances. You have to decide somewhere in your program what concrete instance to create. Factory Method allows you to avoid having the same decision spread out through your client code. If later you want to change the implementation (or e.g. for testing), you have just one place to edit: this may be e.g. a simple global change, through conditional compilation (usually for tests), or even via a dependency injection configuration file.

Be careful about how client code communicates what kind of implementation it wants: that's not an uncommon way of reintroducing the dependencies factories are meant to hide.

score 2 · Answer 4 · answered Feb 14 '11 at 12:58

2

It's not uncommon to see factory member functions in a class, but it makes my eyes bleed. Often their use have been mixed up with the functionality of the named constructor idiom. Moving the creation function(s) to a separate factory class will buy you more flexibility also to swap factories during testing.

answered Feb 14 '11 at 12:58

Johann Gerell

24,991
10
72
122

If my colleague would like, I will suggest to combine this idiom in the factory itself, but the coupling of the modules that I won't even use in a different set of code seems bad to me. – San Jacinto Feb 14 '11 at 13:16

score 1 · Answer 5 · answered Feb 14 '11 at 12:59

When the interface is just for hiding the implementation details and there will be only one implementation of the Base interface ever, it could be ok to couple them. In that case, the factory function is just a new name for the constructor of the actual implementation.

However, that case is rare. Except when explicit designed having only one implementation ever, you are better off to assume that multiple implementations will exist at some point in time, if only for testing (as you discovered).

So usually it is better to split the Factory part into a separate class.

If there is only one implementation of an interface, ever, you don't really need an interface. You could use the PIMPL pattern (though overuse of it has its own drawbacks). — Joris Timmermans, Feb 14 '11 at 13:13

Factory Pattern in C++ -- doing this correctly?

5 Answers5

Linked