[RESOLVED] how can I get rid of these downcasts?

[GeoRanger]

Hi,

The contrived code below demonstrates a problem I am having in a larger program. I want to have a base class which contains the functionality to assemble a group of objects into a container. That functionality can then be shared by derived classes. The objects which get assembled and the container into which they get assembled are very different from one derived class to another, even though the algorithm which assembles them is identical.

Each derived class overrides an abstract method in the base class in order to properly add the objects into their containers. The problem is, I'm having to downcast the objects in the derived class from the abstract types in the base class (a major no-no according to Scott Meyers). I would really like to get rid of the downcasts with some modified architecture. I tried using the "Curiously Recursive Template Pattern" between the base and derived classes, but couldn't see how in the base class to get access to the inner classes defined in the derived class types. I would really like to keep the nested classes encapsulated because the defintions of the objects and their containers make no sense outside of the derived classes.

Is there some way to do this, perhaps some template trick or a design pattern which elegantly addresses what this is trying to accomplish?

Thanks,
GeoRanger

// Base class containing the generic construction algorithm
struct ConstructionWorker
{
    // Abstraction of the building material
    struct XMaterial
    {
        virtual double GetCost() = 0;
    };
    // Abstraction of the container object which holds the material
    struct XBuilding
    {
        virtual double GetTotalCost() = 0;
    };

    virtual XMaterial * GetNewMaterial() = 0;
    virtual XBuilding * GetNewBuilding() = 0;
    // Only the derived classes know how to add their particular material to the container
    virtual void AddMaterialToBuilding(XMaterial *, XBuilding *) = 0;
    // This is the generic algorithm which will work for all derived classes
    XBuilding * ConstructBuilding(void)
    {
        XBuilding * pBuilding = GetNewBuilding();
        for (int i = 0; i < 5; ++i)
        {
            XMaterial * pMaterial = GetNewMaterial();
            AddMaterialToBuilding(pMaterial, pBuilding);
        }
        return pBuilding;
    }
};
struct Mason : public ConstructionWorker
{
    struct XBrick : public XMaterial
    {
        virtual double GetCost() { return 0.50; }
        int GetType() { return 5; }
    };
    struct XBrickWall : public XBuilding
    {
        virtual double GetTotalCost() { return costTotal; }
        std::vector<int> vTypesUsed;
        double costTotal;
    };
    virtual XMaterial * GetNewMaterial() { return new XBrick    (); }
    virtual XBuilding * GetNewBuilding() { return new XBrickWall(); }
    // Only this class knows how to add a brick to a wall, but the downcasts are horrible!
    virtual void AddMaterialToBuilding(XMaterial * pMaterial, XBuilding * pBuilding)
    {
        static_cast<XBrickWall *>(pBuilding)->costTotal += pMaterial->GetCost();
        static_cast<XBrickWall *>(pBuilding)->vTypesUsed.push_back
                                              (static_cast<XBrick *>(pMaterial)->GetType());
    }
};
struct Carpenter : public ConstructionWorker
{
    // etc.
};

[superbonzo]

the problem is that you need so called multimethods but c++ does not have them, take a look here for a Bjarne's take on this issue.

As the paper suggests, a common standard workaround is the visitor pattern, but it's much less versatile a solution as it appears ( eg. generalizing visitors/visitables quickly becomes a PITA ), unless some template machinery is used ( usually, with the help of RTTI , anyway ). Alternatively, you may take a look at existing multimethod emulations for c++ ( see the paper and/or google ).

IMO, either you have a good solid abstraction of the visitor or you change the design to avoid double dispatch altogether.

[OReubens]

1) the totalcost approach in XBuilding is your problem area.

Either the base class needs to have an accumulation of the total (by having the totalCost double as member of the base class instead of in each of the derived classes)
or your XBuilding class needs to have access to the material counts, and individual prices and calculate the price on the fly. Typically by having the vTypes array again as membe rof the base class rather than repeating this in each derived class.

This is a pretty elementary design issue. the current approach requires each derived class to reïmplement either total accumulation and/or keeping track of the materials. There might be a reason for this and you're only showing us a small part of the bigger picture, but even if so, it would make more sense to have a XMaterialCollection which keeps track of the materials list and total cost, and then have the XBuilding either have a (or multiple) materialcollections as a member directly or through a reference/(smart)pointer.

In both cases the "totalcost" isn't going to be virtual at all. it'll either be a regular part of the XBuilding, or XBuilding has access to everything it needs to calculate it (through calling virtual functions).

2) a secondary (related) design issue is that you have a getType in XBrick, but the generic XMaterial is 'untyped'. Typically, I would expect the XMaterial to either have the type member or have it accessible through a virtual.
continuing that line of thought, why does XMaterial::GetCost need to be virtual ? Why not simply:

struct XMaterial
{
    XMaterial(int type, double cost) : type(type), cost(cost) {}

   int getType() const { return type; }
   int getCost() const { return cost; }

private:
   int type;
   double cost;
};

Making getCost (and getType) virtual really only makes sense if and only if their value can ONLY be determined through accessing members that are only available in the derived class and don't exist in base.
In the sample there isn't an indication this is necessary. If you are going to have a cost (and type) implementation in every derived class, then that's a pretty sure indication that they should really be members of the base class.

[razzle]

Is there some way to do this,

It's very simple. In a polymorphic design a base class public interface must be functionally complete. It must sport the virtual methods needed for it to be used in the proper, wanted and expected way.

As it stands there are no methods to add cost and type to XBuilding nor to get the type from XMaterial. Instead this functionality is available in the derived classes XBrick and XBrickWall only with the result that you must downcast to get at it. So "push up" this functionality from the derived classes by exposing appropriate virtual methods at the base class level and the downcasts won't be necessary.

Since inheritance has two major purposes, inheritance of interface and inheritance of implementation, you must be careful. What is the public interface of ConstructionWorker that should be inherited by derived classes such as Mason and Carpenter, and what is the convenience implementation those derived classes are inheriting and supposedly will find useful? Generally the latter, the inheritance of a convenience implementation, should be avoided because it leads to the so called Fragile Base Class Problem.

So I suggest you lift the convenience implementation out of ConstructionWorker (and supply it by some other means to the derived classes). What is left of ConstructionWorker is a public interface and again it should be functionally complete. The ConstructionWorker base class must expose everything that's expected of a ConstructionWorker, be it a Mason or a Carpenter or something else, otherwise you again will end up downcasting. This corresponds to the design: A Mason or a Carpenter is-a ConstructionWorker. Code written using the ConstructionWorker type will work equally well when supplied with Mason or Carpenter objects, all in accordance with the famous Liskov Substitution Principle.

Finally a few technicalities. For polymorphic classes use the class keyword rather than struct. Struct traditionally is used mostly for very simple value classes with little or no abstraction, essentially records.

If you instantiate polymorphically derived classes using the new keyword then their base class must have a virtual destructor. Otherwise if you use delete on a base class pointer this will be an undefined operation and your program will crash (loudly or silently).

The problem is, I'm having to downcast the objects in the derived class from the abstract types in the base class (a major no-no according to Scott Meyers).

And the reason for the no-no is that a downcast may fail at runtime. Constructs where type mismatches cannot be detected already at compiletime should be avoided.

You could improve the situation somewhat by using a dynamic_cast rather than a static_cast. This cast may still fail at runtime but at least you have a chance to check for failure and bail out gracefully if it occurs. But the best of course it to not cast at all.

Strangely there are seasoned programmers who have never heard of Scott Meyers. Just ask superbonzo and OReubens and they'll tell you how useful and indispensable downcasting is.

[GeoRanger]

Thank you three for your responses!


My contrived example doesn't do a great job of expressing the real problem. The real problem is an optimization problem where the derived classes represent requirements from third parties who manage agreements between buyers and sellers. Some of those third-party requirements are very similar in the abstract sense of their implied optimization problem, though the way they rate the material being purchased is completely different from one agreement to another (which is what I tried to imply with the GetCost() and GetType() examples). There's no real way to abstract those differences in the agreements up into the base class. Some of the qualities important to some agreements are negligible in others. I'd have to bring all the qualities cared about by each agreement into the base class as abstract functions and stub them out in derived classes where they don't apply. I'd also have to rework the code when a new kind of agreement is included (in violation of the Open-Closed Principle).


Having said all that, I did actually find a way around the issue, though the resulting code is not necessarily the prettiest. The biggest problem which seemingly requires the downcasting is that only the derived classes know how to insert the XMaterial into the XBuilding (via the abstract function AddMaterialToBuilding). What I realized could be done is to make the entry point to the optimization code (which is ConstructBuilding in the example) into a templated function which takes the derived classes' actual types for XBuilding and XMaterial. In addition, ConstructBuilding also takes one additional argument which is a std::function<> with template arguments which are the same as the template parameters to ConstructBuilding. ConstructBuilding is the main function called by the derived classes so they are able to pass in a function which does the insertion, but now no downcasting is necessary because the types are templated. Once that's done, the abstract functions GetNewMaterial() and GetNewBuilding() are no longer necessary.

template <class TMaterial, class TBuilding>
TBuilding ConstructBuilding(std::function<void(TMaterial const *, TBuilding *)> const & fnAddMaterialToBuilding);

The code had to be reworked quite a bit to move all the effected functions out of a cpp file into header files when they became templates, but I hope the result now falls into the least-of-the-evils category.


Thanks again for the help and the responses. superbonzo, that is an interesting paper at that link. I need to read it when I have more time so I can try to understand it better. I wonder if the proposed changes and syntax will eventually be incorporated into the standard.

Thanks,
GeoRanger

[razzle]

There's no real way to abstract those differences in the agreements up into the base class. Some of the qualities important to some agreements are negligible in others. I'd have to bring all the qualities cared about by each agreement into the base class as abstract functions and stub them out in derived classes where they don't apply. I'd also have to rework the code when a new kind of agreement is included (in violation of the Open-Closed Principle).

I suggest you start by formalizing all aspects of an agreement in an abstract base class called Agreement. There's no way around it. If you don't have that you have no design.

Then making Agreement fully extendible in accordance with the Open-Closed principle becomes a question of solving the so called Expression problem. Here's a solution,

http://i.cs.hku.hk/~bruno/oa/

You'll find a C++ implementation at the end of the link.