Is dynamic_casting through inheritance hierarchy bad practice?

Question

I have got the following data structure:

class Element {
  std::string getType();
  std::string getId();
  virtual std::vector<Element*> getChildren();
}

class A : public Element {
  void addA(const A *a);
  void addB(const B *b);
  void addC(const C *c);
  std::vector<Element*> getChildren();
}

class B : public Element {
  void addB(const B *b);
  void addC(const C *c);
  std::vector<Element*> getChildren();
}

class C : public Element {
  int someActualValue;
}

/* The classes also have some kind of container to store the pointers and
 * child elements. But let's keep the code short. */

The data structure is used to pruduce a acyclic directed graph. The C class acts as a "leaf" containing actual data for algebra-tasks. A and B hold other information, like names, types, rules, my favourite color and the weather forecast.

I want to program a feature, where a window pops up and you can navigate through an already existing structure. On the way i want to show the path the user took with some pretty flow chart, which is clickable to go back in the hierarchy. Based on the currently visited Graph-Node (which could be either A, B or C) some information has to be computed and displayed.

I thought i could just make a std::vector of type Element* and use the last item as the active element i work with. I thought that was a pretty nice approach, as it makes use of the inheritance that is already there and keeps the code i need quite small.

But i have a lot of situations like these:

Element* currentElement;

void addToCurrentElement(const C *c) {
  if(A *a = dynamic_cast<A*>(currentElement)) {
    //doSomething, if not, check if currentElement is actually a B
  }    
}

Or even worse:

vector<C*> filterForC's(A* parent) {
  vector<Element*> eleVec = parent.getChildren();
  vector<C*> retVec;
  for(Element* e : eleVec) {
    if (e.getType() == "class C") {
      C *c = dynamic_cast<C*>(e);
      retVec.append(c);
    }
  }
}

It definitely is object oriented. It definitely does use inheritance. But it feels like i just threw all the comfort OOP gives me over board and decided to use raw pointers and bitshifts again. Googling the subject, i found a lot of people saying casting up/down is bad design or bad practice. I totally believe that this is true, but I want to know why exactly. I can not change most of the code as it is part of a bigger project, but i want to know how to counter something like this situation when i design a program in the future.

My Questions:

1. Why is casting up/down considered bad design, besides the fact that it looks horrible?
2. Is a dynamic_cast slow?
3. Are there any rules of thumb how i can avoid a design like the one i explained above?

Answer 1

There are a lot of questions on dynamic_cast here on SO. I read only a few and also don't use that method often in my own code, so my answer reflects my opinion on this subject rather than my experience. Watch out.

(1.) Why is casting up/down considered bad design, besides the fact that it looks horrible?

(3.) Are there any rules of thumb how i can avoid a design like the one i explained above?

When reading the Stroustrup C++ FAQ, imo there is one central message: don't trust the people which say never use a certain tool. Rather, use the right tool for the task at hand.

Sometimes, however, two different tools can have a very similar purpose, and so is it here. You basically can recode any functionality using dynamic_cast through virtual functions.

So when is dynamic_cast the right tool? (see also What is the proper use case for dynamic_cast?)

• One possible situation is when you have a base class which you can't extend, but nevertheless need to write overloaded-like code. With dynamic-casting you can do that non-invasive.

• Another one is where you want to keep an interface, i.e. a pure virtual base class, and don't want to implement the corresponding virtual function in any derived class.

Often, however, you rather want to rely on virtual function -- if only for the reduced uglyness. Further it's more safe: a dynamic-cast can fail and terminate your program, a virtual function call (usually) won't.

Moreover, implemented in terms of pure functions, you will not forget to update it in all required places when you add a new derived class. On the other hand, a dynamic-cast can easily be forgotten in the code.

Virtual function version of your example

Here is the example again:

Element* currentElement;

void addToCurrentElement(const C *c) {
  if(A *a = dynamic_cast<A*>(currentElement)) {
    //doSomething, if not, check if currentElement is actually a B
  }    
}

To rewrite it, in your base add a (possibly pure) virtual functions add(A*), add(B*) and add(C*) which you overload in the derived classes.

struct A : public Element
{
     virtual add(A* c) { /* do something for A */ }
     virtual add(B* c) { /* do something for B */ }
     virtual add(C* c) { /* do something for C */ }
};
//same for B, C, ...

and then call it in your function or possibly write a more concise function template

template<typename T>
void addToCurrentElement(T const* t)
{
    currentElement->add(t);
}

I'd say this is the standard approach. As mentioned, the drawback could be that for pure virtual functions you require N*N overloads where maybe N might be enough (say, if only A::add requires a special treatment).

Other alternatives might use RTTI, the CRTP pattern, type erasure, and possibly more.

(2.) Is a dynamic_cast slow?

When considering what the majority of answers throughout the net state, then yes, a dynamic cast seems to be slow, see here for example. Yet, I don't have practical experience to support or disconfirm this statement.