Thread: Avoiding downcasts

I need help in better understanding the usual quote "Avoid dynamic casts".
The thing is, all but the simplest hierarchies do not have deriveds providing new interface that is not, and could not, be present in the base class.

Giving the base class no-ops is one of the solutions, however it seems to me it is not very pretty for the interface to provide functionality that is in fact, for all that matters, non-existing. It's like pressing that button on the vendor machine and nothing happens.

What I'm starting to realize is that dynamic casts are in fact a "common" part of any code. Sure, one shouldn't happily cast down, but the advise is perhaps more intended towards performance issues (considering how slow these casts apparently are) and towards those of us who happily cast without at least trying to review their class design.

Is this so? Ot is there a better pattern I can look for that will considerably reduce the need to use dynamic_cast?

>The thing is, all but the simplest hierarchies do not have deriveds providing
>new interface that is not, and could not, be present in the base class.
That's a design issue, not a problem with the advice. Unfortunately, it's also something that has to be dealt with on a case-by-case basis. So if you have a concrete example, I could offer some better advice.

I believe I can give a concrete example. An hierarchy ever present on my program that needed some dynamic casts.

CObject is an ABC defined as such (reduced to most pertinent parts):

Code:

class CObject {
public:
    CObject();
    virtual ~CObject() {}

    virtual bool load(const std::string& id) = 0;
    virtual void show() const = 0;

    int weight() const;
    int volume() const;

protected:
    std::string weight_;
    std::string volume_;
};

CItem, CWeapon, CArmor and CContainer derive from it. Of them CContainer is defined as such:

Code:

class CContainer : public CObject {
private:
    typedef boost::shared_ptr<CObject> CObject_ptr;

public:
    CContainer() {}
    virtual ~CContainer() {}

    virtual bool load(const std::string& id);
    virtual void show() const;

    int maxWeight() const;
    int maxVolume() const;
    int contentsWeight() const;
    int contentsVolume() const;

    // Returns current weight of container (contents plus container weight)
    int currentWeight() const; 
    // Returns current volume of container (contents plus container volume)
    int currentVolume() const; 

private:
    std::vector<CObject_ptr> contents_;
    int maxWeight_;
    int maxVolume_;
    int contentsWeight_;
    int contentsVolume_;

};

CContainer is thus a CObject (bag, chest, ...) that holds CObjects. It can hold CContainers. It's acceptable for a chest to contain a bag that contains other items.

The problem is made apparent in the red section above. A container weight is the sum of its actual weight plus all its contents weight, whereas any other object weight is whatever is returned from CObject->weight(). I'm forced to downcast everytime I need to calculate the overal weight of my Inventory.

>I believe I can give a concrete example.
I believe I can lighten your load, as it were.

>A container weight is the sum of its actual weight plus all its contents weight, whereas
>any other object weight is whatever is returned from CObject->weight().
And the empty weight for a container is returned from weight()? Wouldn't it make more sense for weight() to return the current weight and to have a more specialized interface telling you the empty and maximum weights? If I judge your design correctly, you can do that without changing the base class[1] and avoid downcasting.

[1] Though I would recommend that you make your data members private and provide protected accessors. I would also recommend making weight and volume virtual so that you can customize the behavior without playing games with the data.

Ehhhhhhh..... that example isn't really correct. Virtual functions exist to get around that kind of problem.


The only case in which you should ever need to downcast is when pulling an object out of a generic container.

• Let's say you have a chest which contains a sword (CSword) and a shield (CShield).
• CSword : public CObject, and CShield : public CObject.
• CChest is a container implemented as a vector of CObjects.
• When you put an object into the chest, it is cast up to a CObject.
• When you take an object out of the chest, you have to downcast it.

The problem is... even though downcasting makes it technically feasable to do, it's still a pretty icky situation to deal with. Just creating a new object means going back to every downcast and making sure it's accounted for.

It may be preferable to just deal with everything as the base class.



An illustration of the weight() example solved:

Code:

#include <iostream>

class HasMass {
public:
   HasMass (int mass) : myWeight(mass) { }

   virtual int weight() const { return myWeight; }
private:
   int myWeight;
};

class Weight : public HasMass {
public:
   Weight (int W) : HasMass(W) { }
};

class Container : public HasMass {
public:
   Container() : HasMass(1), contained(0) { }
   virtual int weight() const { return HasMass::weight() + contained->weight(); }
   virtual void add (HasMass * addMe) { contained = addMe; }

private:
   HasMass * contained; 
};

void printMasses (HasMass const & A, HasMass const & B, HasMass const & C) {
   std::cout << A.weight() << " " << B.weight() << " " << C.weight() << '\n';
}

int main (void) {
   Weight oz(1);
   Weight lb(16);
   Container C;

   C.add (&lb);

   std::cout << oz.weight() << " " << lb.weight() << " " << C.weight() << '\n';
   printMasses (oz, lb, C);

   return 0;
}

> And the empty weight for a container is returned from weight()? Wouldn't it make more sense for weight() to return the current weight and to have a more specialized interface telling you the empty and maximum weights?

Hmm... yes that's a possibility alright. Thanks!

> [1] Though I would recommend that you make your data members private and provide protected accessors.

I'm actually on the process of changing that, but for somewhat different reasons. Which leads me to a second question, if you don't mind.

The reason they are protected is that the objects are loaded (without exception) from a file. The pure virtual load() needs write access to the data members when being called from within one of te deriveds. Derived::load() is the only place where the data members defined in the base can be changed.

However I later realized that it is probably best to change the base class design to something like this:

Code:

class CObject {
public:
    CObject();
    virtual ~CObject() {}

    virtual bool load(const std::string& id) = 0;
    virtual void show() const = 0;

    int weight() const;
    int volume() const;

private:
    std::string weight_;
    std::string volume_;
    /* ... there's some more .. */
};

bool CObject::load(const std::string& id) {/*... do base::load() here ...*/}

This way I can call CObject::load() from within my deriveds load() before moving on to load derived specific data. And the bonus is that I will still avoid an interface with write access (no need to provide write access assessors). But it comes at a cost...

The load() process searches for an item ID on a data file. Once found that line is extracted onto a string and the filestream is closed. From there on I work on the string extracting all the information to fill in the object instance. So, this last class definition will force me to also provide:

Code:

class CObject {
/* ... */
protected:
std::string buffer_; // string containing object info. Data already processed was removed.
/*... */
};

Question is... I'm not sure if i'm that happy storing a data member that will have absolutely no use once load() finishes. Is this acceptable? Should I make it static instead?

Originally Posted by QuestionC

Ehhhhhhh..... that example isn't really correct. Virtual functions exist to get around that kind of problem.

I considered that possibility, QuestionC. However, that's always my last resort. To provide another hierarchy structure will indeed solve my problem. But it will come at the cost of yet another coupling and more complexity.

Fortunately, CObjects in my code are under very tight control. They are owned by a class CInventory that acts as a container of objects. CPlayer, CMonster, CRoom, are examples of classes that have one or more CInventory's. Objects are created and destroyed inside CInventory. Nothing is done to them outside that class scope. And I mean nothing! So, I have a, if not perfect, at least, more informed idea of how much dynamic_cast can downgrade my code quality when compared to other options.

I do agree with that pattern QuestionC. Don't take me wrong. But if I'm faced with a situation where dynamic_cast or judicious class design proves to solve my problem, I will choose that in face of yet another coupling mechanism and more complexity. Otherwise I will soon be overwhelmed by my own project OOP design.

>Is this acceptable?
Not in my opinion. It seems like load is an issue because it's trying to do too much across multiple objects. How about breaking it up into load and fill, where load obtains a buffer from the file (keyed on the id) and fill actually populates the data members of your object? That should be easier to work with.

Devide and conquer. Will try that. Thanks again.

I considered that possibility, QuestionC. However, that's always my last resort. To provide another hierarchy structure will indeed solve my problem. But it will come at the cost of yet another coupling and more complexity.

A new heirarchy structure?

All I was suggesting is you take the weight method of Object (allready a pure virtual class), and make it virtual. Pretty much nothing changes except that you can now implement containers without a not-needed dynamic cast. I don't really understand where the new heirarchy gets created.

I read again your post. The HasMass and Weight classes threw me away from its intent. So, yes. I missread your solution. Sorry about that.

Prelude had suggested just that before, which only added to my confusion.