Should I change my design to prevent dynamic casts?

Question

I have read several threads about dynamic casts in C++, all full of people claiming it indicates bad design. In other languages I never gave it much thought when checking the type of an object. I never use it as an alternative to polymorphism and only when strong coupling seems perfectly acceptable. One of these situations i encounter quite often: having a list (i use std::vector in C++) of objects, all derived from a common base class. The list is managed by an object that is allowed to know the different subclasses (often it's a small hierarchy of private classes within the managing objects class). By keeping them in a single list (array, vector, ..) I can still benefit from polymorphism, but when an operation is meant to act on objects of a specific subclass I use a dynamic cast or something similar.

Is there a different approach to this type of problem without dynamic casts or type checking that I am missing? I am really curious how programmers that avoid these at all costs would handle them.

If my description is too abstract I could write a simple example in C++ (Edit: see below).

class EntityContacts {
private:
  class EntityContact {
  private:
    virtual void someVirtualFunction() { };            // Only there to make dynamic_cast work
  public:
      b2Contact* m_contactData;
  };

  class InternalEntityContact : public EntityContact {
  public:
    InternalEntityContact(b2Fixture* fixture1, b2Fixture* fixture2){
        m_internalFixture1 = fixture1;
        m_internalFixture2 = fixture2;
    };

    b2Fixture* m_internalFixture1;
    b2Fixture* m_internalFixture2;
  };

  class ExternalEntityContact : public EntityContact {
  public:
    ExternalEntityContact(b2Fixture* internalFixture, b2Fixture* externalFixture){
        m_internalFixture = internalFixture;
        m_externalFixture = externalFixture;
    };

    b2Fixture* m_internalFixture;
    b2Fixture* m_externalFixture;
  };

  PhysicsEntity* m_entity;
  std::vector<EntityContact*> m_contacts;
public:
  EntityContacts(PhysicsEntity* entity)
  {
    m_entity = entity;
  }

  void addContact(b2Contact* contactData)
  {
    // Create object for internal or external contact
    EntityContact* newContact;
    if (m_entity->isExternalContact(contactData)) {
        b2Fixture* iFixture;
        b2Fixture* eFixture;
        m_entity->getContactInExFixtures(contactData, iFixture, eFixture);
        newContact = new ExternalEntityContact(iFixture, eFixture);
    }
    else
        newContact = new InternalEntityContact(contactData->GetFixtureA(), contactData->GetFixtureB());

    // Add object to vector
    m_contacts.push_back(newContact);
  };

  int getExternalEntityContactCount(PhysicsEntity* entity)
  {
    // Return number of external contacts with the entity
    int result = 0;
    for (int i = 0; i < m_contacts.size(); ++i) {
        ExternalEntityContact* externalContact = dynamic_cast<ExternalEntityContact*>(m_contacts[i]);
        if (externalContact != NULL && getFixtureEntity(externalContact->m_externalFixture) == entity)
            result++;
    }
    return result;
  }
};

It is a simplified version of a class that i use for collision detection in a game that uses box2d physics. I hope that the box2d details don't distract too much from what i am trying to show. I have a very similar class 'Event' that creates different types of event handlers which is structured in the same way (with subclasses of a base class EventHandler instead of EntityContact).

Answer 1

At least from my perspective, dynamic_cast exists for a reason, and there are times it's reasonable to use it. This may be one of those times.

Given the situation you describe, one possible alternative may be to define more of the operations you need in the base class, but define them as (possibly silently) failing if you invoke them for the base class or other classes that don't support those operations.

The real question is whether it makes sense to define your operations this way. Going back to the typical animal-based hierarchy, if you're working with Birds, it's often sensible for the Bird class to define an fly member, and for the few birds that can't fly, just have it fail (theoretically should be renamed as something like attempt_to_fly, but that rarely accomplishes much).

If you're seeing a lot of this, it tends to indicate a lack of abstraction in your classes -- for example, instead of a fly or attempt_to_fly, you might really want a travel member, and it's up to the individual animal to determine whether to do that by swimming, crawling, walking, flying, etc.

Answer 2

This

but when an operation is meant to act on objects of a specific subclass I use a dynamic cast or something similar

sounds like the object modelling isn't correct. You have a list contain subclass instances, but they're not really subclasses since you can't act on them all in the same fashion (Liskov etc.).

One possible solution is to extend your base class such that you have a set of no-op methods that certain subclasses can override. But that still doesn't sound quite right.

Answer 3

The question is quite broad, so some general points to consider:

• It's a feature of the language for a reason, meaning there are valid use cases.
• It's no inherently good or bad in general.
• Check if it the right solution for your problem.
• Know the alternatives to solve your problem.
• Comparing it with other languages should include the question whether or not the alternatives of C++/C++11 are possible in that other language as well.
• dynamic_cast yields a certain cost. If it's about run-time performance, check it against the cost of the alternatives.
• Checks are performed at run-time, which yields a certain risk that you deliver buggy software if it is not tested properly. With static type checking, the compiler can help you, even guarantee, that certain problems/bugs won't occur.

Answer 4

Usually in such situations you would manage not a list of derived objects but a list of interface pointers (or base pointers) which refer to the actual derived objects. Whenever you want to perform an operation on a specific object, you access it via its interface/base class that should expose all the required functionality. The derived classes should override the exposed base functionality with a specific implementation.

Answer 5

use of dynamic_cast is the symptom of wanting to implement a design pattern like Visitor or Command, so i would recommend refactoring to make this apparent

Answer 6

As with any approach that is considered bad programming, there are always going to be some exceptions. When a programmer says "such and such is evil and you should never do it", they really mean "there's almost never a reason to use such and such and so you better be able to explain yourself." I, myself, have never encountered a situation in which a dynamic_cast is absolutely necessary and can't be easily refactored. However, if it gets the job done, so be it.

Since we don't know your specific problem, I can only give advice given what you've told us. You say have a container of polymorphic base types. For example:

std::vector<Base*> bases;

If you're going to be using any of the objects that you place into this container in a derived-specific way, then its not really a container of base objects, is it? The whole point of having a container of pointers to Base objects is that you can iterate over them and treat them all as Base objects. If you have to dynamic_cast to some Derived type, then you've abused the polymorphic behaviour of Base*s.

If Derived inherits from Base, then Derived is-a Base. Simply put, if you use a polymorphic Base pointer to a Derived type, then you should only be using the features of Derived that make it a Base.