Is “Factory Method” An Anti-Pattern?

Let’s take another look at this version of the “two implementations, one interface” code from that one about portability:

// Event.h v8
class Event {
public:
  virtual ~Event() {}
  virtual void Wake() = 0;
  ...
};

std::auto_ptr<Event> CreateEvent();

// Event.cpp
std::auto_ptr<Event> CreateEvent()
{
  ... return whichever derived class of Event is appropriate ...
}

What I didn’t say at the time is that this is sort-of the Factory Method pattern, though a strict following of that pattern would instead have us put the CreateEvent function inside the class as a static member, Event::Create(). And the pattern also includes designs where CreateEvent is a factory method on a different class from Event, but it‘s specifically “self-factory methods” such as Event::Create that I’m concerned with here.

(As an aside: the patterns literature comes in for a lot of criticism for being simplistic. Which it is: the GoF book could be a quarter the size if it engaged in less spoon-feeding and pedagoguery. (And by pedagoguery I mean pedagoguery.) But in a sense the simplicity and obviousness of the patterns they’re describing is the whole point: thanks to the patternistas (patternauts?), a lot of simple and obvious things that often come up in programmers’ natural discourse about code, and that didn’t previously have names, now do. Reading a patterns book might not make your code much better unless you’re a n00b, but that’s not what it’s for. It’s for making your discourse about code better. In any n>1 team, being able to discourse well about code is a vital skill.)

But what I also didn’t say at the time, is that whether CreateEvent is a member or not, so long as it’s in event.h, this code appears to have cyclic dependencies — to be a violation of the principle of “levelisability” set out in the Lakos book.

What’s going on, as you can see on the left, is that, although the source file dependencies themselves don’t exhibit any cycles, viewing, as Lakos does, each header and its corresponding .cpp file as forming a component — the three grey rectangles — produces a component dependency graph with cycles: win32/event ↔ event ↔ posix/event.

One way around that would be to move CreateEvent out into its own component — a freestanding event factory — as seen on the right. With this change, the design is fairly clearly levelisable at both the file level and the component level. This refactoring is an example of what Lakos (5.2) calles escalation: knowledge of the multifarious nature of events has been kicked upstairs to a higher-level class that, being higher-level, is allowed to know everything. (The file event.cpp now gets a question-mark because, as the implementation file for what may now be a completely abstract class, it might not exist at all — or it might exist and contain the unit tests.)

But is it worth it? We’ve arguably complicated the code — requiring users of events to know also about event factories — for what’s essentially the synthetic goal of levelisability: synthetic in the sense that it won’t be appearing in any user-story. Any subsequent programmer working on the code would be very tempted to fold the factory back into event.h under the banner of the Factory Method pattern.

Moreover, in this case the warning is basically a false positive: if Event is truly an abstract class (give or take its factory method), then the apparent coupling between, say, posix/event and event is not in fact present: posix/event can be unit-tested without linking against event.o nor win32/event.o. (Not that, in this particular example, posix/event and win32/event would both exist for the same target — but factory methods obviously also get used for cases where both potential concrete products exist in the same build.) Though conversely, if Event had any non-abstract methods — any with implementations in event.cpp — then it’d be a true positive not a false positive, as all the different event types would be undesirably link-time coupled together.

One reason that the refactoring is worth it, is the same sort of reason that fixing compiler warnings, i.e. altering the code so they don’t trigger, is worth it, even in instances when the warning doesn’t point out a bug: because if you let warnings proliferate, real ones will get lost in the noise, and ideally you aim for the zero-warnings state in order that the introduction of any new warning is an easily-visible alert telling you that there’s a new potential bug to check for. Steve Maguire is talking here about unit tests, but the same applies to compiler warnings: “[W]hen they corner a bug, they grab it by the antennae, drag it to the broadcast studio, and interrupt your regularly-scheduled program”.

Exactly like compiler warnings, cyclic-dependency warnings — which are really design warnings — are sometimes false positives, but likewise it’s worth aiming for the “zero design warnings” state, because it makes new design warnings stand out so. I ran a cycle-checker script (in the spirit of the ones in Lakos) over my own Chorale project, and the result was that it effectively shone a spotlight on all the parts of the code where the design was a bit icky. Every cycle it produced — one was dvb::Service ↔ dvb::Recording, another was all the parts of db::steam depending on each other in a big loop — was a place where I’d at some time thought, “Hmm, this isn’t quite right, but it works for now and I’ll come back and do it properly”. And of course without anything to remind me, I never had gone back and done it properly.

So it turns out that you can’t have both factory methods and levelisability. You have to pick one or the other. And levelisability is better.