1

In a project I'm working on in C++, I need to create objects for messages as they come in over the wire. I'm currently using the factory method pattern to hide the creation of objects:

// very psuedo-codey
Message* MessageFactory::CreateMessage(InputStream& stream)
{
    char header = stream.ReadByte();

    switch (header) {
    case MessageOne::Header:
        return new MessageOne(stream);
    case MessageTwo::Header:
        return new MessageTwo(stream);
    // etc.
    }
}

The problem I have with this is that I'm lazy and don't like writing the names of the classes in two places!

In C# I would do this with some reflection on first use of the factory (bonus question: that's an OK use of reflection, right?) but since C++ lacks reflection, this is off the table. I thought about using a registry of some sort so that the messages would register themselves with the factory at startup, but this is hampered by the non-deterministic (or at least implementation-specific) static initialization order problem.

So the question is, is it possible to implement this type of factory in C++ while respecting the open/closed principle, and how?

EDIT: Apparently I'm overthinking this. I intended this question to be a "how would you do this in C++" since it's really easy to do with reflection in other languages.

Neil Williams
  • 12,318
  • 4
  • 43
  • 40
  • It's easy with python too because you can read string (like message tag/id) and lookup a class name in your file and then instantiate the class from which you can instantiate objects. I don't think there's an 'easy' way to do it in C++ though. – stefanB May 12 '09 at 23:18
  • I don't see why static initialization order would prevent a registry. First, it doesn't have to (and probably shouldn't be) static, and second, even if it is, the initialization order only becomes a problem if it is accessed from other statics. And if it is, you have far greater problems (overuse of globals) than this. – jalf May 13 '09 at 11:44
  • What I meant is that you couldn't do something akin to what epatel suggests without using a singleton or relying on static initialization order. But it's all moot now since it's overengineering. – Neil Williams May 13 '09 at 15:41

5 Answers5

2

I think that the open/closed approach and DRY are good principles. But they are not sacred. The goal should be making the code reliable and maintainable. If you have to perform unnatural acts to adhere to O/C or DRY, then you may simply be making your code needlessly more complex with no material benefit.

Here is something I wrote a few years ago on how I make these judgment calls.

Foredecker
  • 7,395
  • 4
  • 29
  • 30
1

You could convert classes that create messages (MessageOne, MessageTwo ...) into message factories and register them with top level MessageFactory on initialization.

Message factory could hold map of MessageX::Header -> instance of MessageXFactory kind of map.

In CreateMessage you would find instance of MessageXFactory based on message header, retrieve the reference to MessageXFactory and then call it's method that would return instance of the actual MessageX.

With new messages you no longer have to modify the 'switch', you just need to add an instance of new MessageXFactory to the TopMessageFactory.

example:

#include <iostream>
#include <map>
#include <string>

using namespace std;

struct Message
{
    static const int id = 99;
    virtual ~Message() {}
    virtual int msgId() { return id; }
};

struct NullMessage : public Message
{
    static const int id = 0;
    virtual int msgId() { return id; }
};

struct MessageOne : public Message
{
    static const int id = 1;
    virtual int msgId() { return id; }
};

struct MessageTwo : public Message
{
    static const int id = 2;
    virtual int msgId() { return id; }
};

struct MessageThree : public Message
{
    static const int id = 3;
    virtual int msgId() { return id; }
};

struct IMessageFactory
{
    virtual ~IMessageFactory() {}
    virtual Message * createMessage() = 0;
};

struct MessageOneFactory : public IMessageFactory
{
    MessageOne * createMessage()
    {
        return new MessageOne();
    }
};

struct MessageTwoFactory : public IMessageFactory
{
    MessageTwo * createMessage()
    {
        return new MessageTwo();
    }
};

struct TopMessageFactory
{
    Message * createMessage(const string& data)
    {
        map<string, IMessageFactory*>::iterator it = msgFactories.find(data);
        if (it == msgFactories.end()) return new NullMessage();

        return (*it).second->createMessage();
    }

    bool registerFactory(const string& msgId, IMessageFactory * factory)
    {
        if (!factory) return false;
        msgFactories[msgId] = factory;
        return true;
    }

    map<string, IMessageFactory*> msgFactories;
};

int main()
{
    TopMessageFactory factory;
    MessageOneFactory * mof = new MessageOneFactory();
    MessageTwoFactory * mtf = new MessageTwoFactory();

    factory.registerFactory("one", mof);
    factory.registerFactory("two", mtf);

    Message * msg = factory.createMessage("two");
    cout << msg->msgId() << endl;

    msg = factory.createMessage("one");
    cout << msg->msgId() << endl;
}
stefanB
  • 77,323
  • 27
  • 116
  • 141
  • But that just replaces switching on message type with a map that required registering of each message type... still violates the open/closed principle. – Neil Williams May 12 '09 at 22:47
  • @Neil: No it does not violate the open/close. Because you can add more types by loading a new DLL (shared Library). As the DLL load it registers the message constructor with the factory map. Thus you can dynamically expand the application by just dropping a DLL into the application directory. – Martin York May 12 '09 at 22:54
  • @Martin: The answer never mentions DLLs, even in its updated form. @stefan: The duplication has now just moved to the factory.registerFactory lines. – Neil Williams May 12 '09 at 23:04
  • @Neil: Yeah that's right. I would say that it reflects the idea of open/closed principle. I'm not dll expert but I would imagine that you would look for a specific file names in a directory (e.g. MessageX.dll or libMessageX ), load up all libraries that match the description, instantiate them and register with the factory. – stefanB May 12 '09 at 23:12
  • @Neil: It does not need to be a DLL. The principle is that you do not need to change the factory method (ie closed to change). But new implementations can be added dynamically simply by registering new version into the factory map (ie open for expansion [or whatever the term is]) :-) – Martin York May 12 '09 at 23:17
  • @stefan: Ideally, to add a new message, I'd just create a new class and drop it in; no changes elsewhere in the code, as per epatel's answer. On a side note, I'd consider DLLs way overkill here since I was trying to reduce the amount of code I had to write (and aren't really specific to C++ anyway). – Neil Williams May 12 '09 at 23:18
  • @Martin: but the point is you now have to modify something just to add a class, whether it's the factory or not isn't the point. – Neil Williams May 12 '09 at 23:20
1

You do not need to make your code follow every possible principle simultaneously. The aim should be to stick to as many of those paradigms as possible and no more. Do not over-engineer your solution -- you are likely to end up with spaghetti code otherwise.

dirkgently
  • 108,024
  • 16
  • 131
  • 187
1

I have answered in another SO question about C++ factories. Please see there if a flexible factory is of interest. I try to describe an old way from ET++ to use macros which has worked great for me.

The method is macro based and are easily extensible.

ET++ was a project to port old MacApp to C++ and X11. In the effort of it Eric Gamma etc started to think about Design Patterns

Community
  • 1
  • 1
epatel
  • 45,805
  • 17
  • 110
  • 144
0

First, your system is not so open ended, since you switch on an 8-bit char, so your message type count won't exceed 256 ;-)

Just joking apart, this is a situation I'd use a little templated factory class (stateless if you put your char message type in a non-class template arg, or with just that char as state) that accepts your stream& and does the new on its T template arg, passing the stream& and returning it. You'll need a little registrar class to declare as static with global scope, and register the concrete T-instantiated factory (via an abstract base class pointer) with a manager (we have a generic one that takes a "factory domain" key). In your case I wouldn't use a map but directly an 256 "slot" array to put the factory_base* in.

One you have the factory framework in place, it's easy and reusable. --DD

ddevienne
  • 1,802
  • 2
  • 17
  • 28