Problem using abstract factory..

Question

Hi, I am using an abstract factory to create user interface components such as dialogs. The abstract factory used is returned from a currently selected generic "INode" which is the base class for several different types of node. So for instance, if I want to add a new node of the same type as the selected node, the scenario goes something like this:

(please note this is semi-pseudo code)

User clicks node and the node gets stored for later use:

void onTreeNodeSelected(INode *node)
{
    selectedNode = node;
}

User clicks "add" on the user interface:

void onAddClicked()
{
    IFactory *factory = selectedNode->getFactory();
    Dialog *dialog = factory->createAddDialog(parentWidget);
    dialog->show();
}

Which all seems fine. The problem comes when I want to edit the selected node:

void onEditClicked()
{
    IFactory *factory = selectedNode->getFactory();
    Dialog *dialog = factory->createEditDialog(selectedNode, parentWidget);
    dialog->show();
}

Oh dear.. I'm passing in an INode object. At some point I'm going to have to downcast that to the correct node type so the dialog can use it properly.

I've studied the "PostgreSQL Admin 3" source code, and they do something similar to this. They get round it by doing something like this:

FooObjectFactoryClass::createDialog(IObject *object)
{
    FooObjectDialog *dialog = new FooObjectDialog((FooObject*)object);
}

Yeck.. cast!

The only way I can think around it and still able to use my factories is to inject the node itself into the factory before it is returned:

FooNode : INode
{
    FooNodeFactory* FooNode::getFactory()
    {
        fooNodeFactory->setFooNode(this);
        return fooNodeFactory;
    }
}

So then my edit event can do this:

void onEditClicked()
{
    IFactory *factory = selectedNode->getFactory();
    Dialog *dialog = factory->createEditDialog(parentWidget);
    dialog->show();
}

And it will use the injected node for context.

I suppose if there is no injected code, the createEditDialog could assert false or something.

Any thoughts?

Thanks!

Answer 1

In my opinion, using C-style casts (although C++ style would be preferred) is perfectly acceptable as long as your code is properly commented.

I'm not a big fan of DI (dependency injection) because it makes some code hard to follow and, in your case, I would rather look at a dynamic_cast<>() or something than try to follow injected code over multiple source files.

Answer 2

I would sugest two things.

First: there is nothing wrong with casting. If you want to be safe, you could use RTTI (type_id stuff) or some virtual functions in the INode class which could return some info which would let you know if it is safe to cast.

Second: you could check to see what the createEditDialog function needs, and put those in virtual functions in either INode, or an inherited class which would be the type createDialog expects.

In general, I see nothing really wrong with the problem you describe, and it is hard to give more suggestions without seeing the whole code, which I assume is unfeasible.

Answer 3

A common solution is "double-dispatch", where you call a virtual function on one object, which in turn calls a virtual function on the other, passing this, which now has the correct static type. So, in your case, the factory can contain "create" functions for the various types of dialogues:

class IFactory
{
public:
    ....
    virtual Dialog* createEditDialog(ThisNode*, IWidget*);
    virtual Dialog* createEditDialog(ThatNode*, IWidget*);
    virtual Dialog* createEditDialog(TheOtherNode*, IWidget*);
    ....
};

then each type of node has a virtual createEditDialog that dispatches to the correct factory function:

class INode
{
public:
    ....
    virtual Dialog* createEditDialog(IWidget* parent) = 0;
    ....
};

class ThisNode : public INode
{
public:
    ....
    virtual Dialog* ThisNode::createEditDialog(IWidget* parent)
    {
        return getFactory()->createEditDialog(this, parent);
    }
    ....
};

Then you can create the correct dialogue as

void onEditClicked()
{
    Dialog *dialog = selectedNode->createEditDialog(parentWidget);
    dialog->show();
}

Answer 4

Your approach of injecting the node into the factory is generally a pattern I've found useful, but there are often times when you don't have a reference to the target object when you're creating the factory like you do here. So this might work fine for you in this case and it's simpler than handling this kind of problem in the general case.

For the more general case, you need to work with the notion of interfaces and establish a mechanism by which your INode object can publish which interfaces it supports and provide access to those interfaces for clients. Doing this completely dynamically leads to a COM-like approach that requires dynamic registration and casting. But you can also do this in a statically typed manner if you have a relatively stable set of interfaces you want to expose and can afford to edit the INode interface when you need to add a new component interface.

So this would be an example of how to do the simple statically typed approach:

struct INode
{
     virtual INodeSize* getNodeSizeInterface() = 0;

     virtual INodeProperties* getNodePropertiesInterface() = 0;

     virtual INodeColor* getNodeColorInterface() = 0;

     ... // etc
}

Now each INode implementation can return some or all of these component interfaces (it would just return NULL if it didn't implemen them). Then your dialogs operate on the component interfaces to do their work instead of trying to figure out which actual implementation of INode was passed in. This will make for much more flexible mapping between dialogs and node implementations. A dialog can quickly determine whether it has a "compatible" INode object by verifying that it returns a valid object for each interface that the dialog is interested in.

Answer 5

I think a cast inside createEditDialog is not a bad thing in this case, even though you give up compile time checks. If the type of the node does not change at runtime, you could use templates instead of an abstract INode-class.

Otherwise, your proposed solution is the one that I would think of as well. I would, however, rename the Method to something like "getSelectedNodeDialogFactory" (i know, long name) so that it is clear that the factory returned is specific to that node. Are there other dialogs that need to know the concrete type of the INode object? Does createAddDialog need a parent or a predecessor node, maybe? Those could all go in the factory-with-selected-node class.