Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have been writing an event class for my game engine and I came across to the following problem:

Is casting a base class object to a derived class object given a type flag a good programming design?

Let me give you the following simple example. I have this enum holding the categories of the events:

enum DisplayEventType
{
    DET_UNSET,
    DET_WINDOW_EVENT,
    DET_KEYBOARD_EVENT,
    DET_MOUSE_EVENT,
    DET_JOYSTICK_EVENT
};

This is the base class of all the events:

typedef std::unique_ptr<DisplayEventData> DisplayEventDataMP;

class DisplayEvent
{
    private:

        // The type of the display event
        DisplayEventType mType;

        // The data of the event
        DisplayEventDataMP mEventData;

    public:

        // Constructor
        DisplayEvent(DisplayEventType type = DisplayEventType::DET_UNSET, DisplayEventDataMP eventData = std::unique_ptr<DisplayEventData>((DisplayEventData*) 0));

        // Retrieves the event type of the current event
        DisplayEventType GetType() const;

        // Sets the event type of the current event
        void SetType(DisplayEventType type);

        // Retrieves a raw pointer to the event data of the current event
        DisplayEventData* GetEventData() const;

        // Sets the event data of the current event
        void SetEventData(DisplayEventDataMP eventData);
};

This is the base class of the event data:

class DisplayEventData
{
    public:

        // Virtual destructor, for the deallocation of the interface implementors to work
        virtual ~DisplayEventData();
};

And these are the derived classes

class WindowEventData : public DisplayEventData
{
    // Window event data specific data and methods
};

class KeyboardEventData : public DisplayEventData
{
    // Keyboard event data specific data and methods
};

class MouseEventData : public DisplayEventData
{
    // Mouse event data specific data and methods
};

class JoystickEventData : public DisplayEventData
{
    // Joystick event specific data and methods
};

Some basic usage would be this:

// Poll for next event in the queue
DisplayEvent ev;
display->PollEvent(ev); // Fills ev with the needed data

if (ev.GetType() == DisplayEventType::DET_KEYBOARD_EVENT)
{
    KeyboardEventData* kev = static_cast<KeyboardEventData>(ev.GetEventData());
    // Do stuff
}

Is the above design viable/correct? If not what would a better alternative be in terms of object oriented or even generic programming?

share|improve this question
    
I think a pattern like this is common when posting events. Some thoughts: 1) Perhaps SetType and SetEventType could be private. It looks like it is only called from the base class constructor. 2) Why have the defaults on the constructor parameters? Instead, consider forcing the derived classes to provide that value since it really is required, unless there is some specific need for a default constructor. See RAII. 3) Could RTTI be a better solution here? –  Moby Disk Jul 7 at 17:50
3  
Hi! Welcome to CR! Please note that example code is off topic here. I think your question is ok, but it is flirting with it. –  ckuhn203 Jul 7 at 18:02

5 Answers 5

Is casting a base class object to a derived class object given a type flag a good programming design?

Usually no.
It usually indicates a badly defined interface in your base class.

A better way to do it will depend on what you want you want to do with it. If you can define an abstract interface with virtual functions that would be the normal way of solving this problem.

But you seem to be using DisplayEventData as simply a bag of properties so maybe there is no generic interface. In which case I would go with a union (but that choice could change quickly based on other information).

struct DisplayEventData   // struct because it is a property bag not a real class.
{
     DisplayEventType   type;
     union
     {
            WindowEventData   win;
            KeyboardEventData key;
            MouseEventData    mouse;
            JoystickEventData joy;
     } data;
};

PS:

This is the wrong cast:

KeyboardEventData* kev = static_cast<KeyboardEventData>(ev.GetEventData());

You should be using dynamic_cast

share|improve this answer
    
static_cast<> should work if he's doing the check manually no? –  sircodesalot Jul 7 at 17:58
    
No. static_cast<> works for casting towards the base class as there is no ambiguity (and can be done at compile time). You need to use dynamic_cast<> when casting away from the base class as it is dynamic and depends on actual runtime types. (I would bet (though am not usre) it is undefined behavior to do otherwise). –  Loki Astari Jul 7 at 18:01
    
Good to know, thanks! –  sircodesalot Jul 7 at 18:02
    
Isn't the usage of unions far from the OO concepts? (Although it still seems way better than my design) –  TheArtist Jul 8 at 6:14
    
@TheArtist: If you have OO objects then use objects. If you have multiple potential property bags then unions seems a natural solution. C++ is a multi-paradigm language; don't throw away one paradigm because it does not fit with your current notions of how things should work. A tool is a tool. learn to use each at the appropriate place. –  Loki Astari Jul 8 at 17:21

I think I'd be inclined to use a std::unique_pointer as the type of thing returned by the poll and use polymorphism rather than enums. That is,

// Poll for next event in the queue
std::unique_ptr<DisplayEvent> ev{display->PollEvent()};   
ev->doStuff();   // polymorphic call to overloaded doStuff member

This avoids both object slicing and ugly enum/switch construction.

Update

To flesh out this idea a bit more, let's play with animals. I'm hoping that how this is useful will be apparent, but I'm using more concrete names and actions to make it perhaps a bit easier to understand and more general. This code starts with a base class Animal which is analogous to your event base class.

#include <memory>
#include <iostream>
#include <string>

class Animal 
{
    static int serial;
    int id;
public:
    Animal() : id(++serial) {};
    virtual int getId() const { return id; }
    virtual std::string speak() const { return "I cannot speak!"; }
    virtual ~Animal() { std::cout << "goodbye from animal #" << id << '\n'; }
    friend std::ostream &operator <<(std::ostream &out, const Animal& a) {
        return out << a.speak() << '\n';
    }
};

int Animal::serial = 0;

I've added a serial number so that each animal gets a unique id so we can easily tell which instance of a class is doing what.

Next are a few other animal classes, each of which are specializations of the base type.

class Cow : public Animal
{
public:
    Cow() {}
    virtual ~Cow() { std::cout << "~Cow " << getId() << "\n"; }
    std::string speak() const { return "moo!";}
};

class Cat : public Animal
{
public:
    Cat() {}
    virtual ~Cat() { std::cout << "~Cat " << getId() << "\n"; }
    std::string speak() const { return "meow!";}
};

class Dog : public Animal
{
public:
    Dog() {}
    virtual ~Dog() { std::cout << "~Dog " << getId() << "\n"; }
    std::string speak() const { return "woof!";}
};

Now we have a function which returns a pointer to an Animal but, like your event polling loop, we don't know which kind of animal it will be.

Animal* getCritter(int type)
{
    if (0 == type % 3) 
        return new Dog();
    if (0 == type % 5)
        return new Cow();
    return new Cat();
}

Here's the main routine to exercise all of this

int main() 
{
    for (int i=0; i < 12; ++i) {
        std::unique_ptr<Animal> c{getCritter(i)};
        std::cout << "animal #" << c->getId() << " says " << *c;
    }
}

Note that I've used std::unique_ptr<Animal> here. I could just as easily have used a plain pointer here, but std::unique_ptr<> has a few advantages in that the destructor automatically gets called when each Animal goes out of scope (probably the same way you've got no reason to keep already-processed events around) and that I avoid inadvertently making spurious copies of objects because it's actively prevented by std::unique_ptr.

In this case the base class has a friend to create an overload for operator<< in the same way you might have a generic base class member function that processes each event. The base class function calls the overloaded speak() function that gives each animal its own individual unique sound in the same way that different kinds of events will each need to be processed, but each with its own particular routines and data structures.

share|improve this answer
    
I so far like this approach the most because it avoids dynamic_cast that seems to be used to equally bad designed circumstances like mine. The problem is that i still don't know how i could handle different type of Events that would be returned –  TheArtist Jul 8 at 6:12
    
@TheArtist: I've added further explanation that I'm hoping will make it easier for you to see how to apply this technique in your own code. –  Edward Jul 8 at 19:48
    
Ok i get it, so in my case the Events must have their own handlers? And what about the specific event data? How will i pass them? –  TheArtist Jul 8 at 22:25
    
Yes, events would have their own handlers. Event-specific data can be part of the event objects themselves in the same way that the animal-specific speak() functions are part of each Animal interface. So a KeyboardEvent might have a small bit of data identifying which key was pressed, and a WindowEvent might contain a large amount of data containing the backing store. Because the data (and handlers) are all specific to the type of event, it makes sense to have those things be part of the specific event class. –  Edward Jul 9 at 0:42

You may consider looking into the dynamic_cast keyword. which allows polymorphic classes to be down-casted based on their type.

For example:

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

class Derived : public Base {

};

int main() {
    Base* base = new Base;
    Base* derived = new Derived;

    cout << dynamic_cast<Derived*>(base) << endl;     // Prints "0" or "nullptr"
    cout << dynamic_cast<Derived*>(derived) << endl;  // Prints the address of the object.
}

This will work because if the base class is polymorphic (has some sort of virtual function) the compiler emits information that makes it possible for dynamic_cast to check what type the object actually is. If it is the correct type, it will perform the cast. If not it will not (returning nullptr).

That said, you should never use switch/cascading if statements with polymorphism. It's better to declare a virtual method that you override in your classes that perform the task instead.

share|improve this answer

Casting a base class object to a derived class object is usually not desired design. Though, it is commonly acceptable as a shortcut approach.

You can consider using Double dispatch and Visitor pattern to handle your event loop. That will be more performance efficient, though is not the same obvious as mentioned shortcut.

class EventDataVisitor
{
public:
    virtual void visit(WindowEventData& event) {};
    virtual void visit(KeyboardEventData& event) {};
    virtual void visit(MouseEventData& event) {};
};

class DisplayEventData
{
public:
    virtual void accept(EventDataVisitor& visitor) = 0;
    virtual ~DisplayEventData();
};

class WindowEventData : public DisplayEventData
{
    virtual void accept(EventDataVisitor& visitor){ visitor.visit(*this); }
};

class KeyboardEventData : public DisplayEventData
{
    virtual void accept(EventDataVisitor& visitor){ visitor.visit(*this); }
};

class MouseEventData : public DisplayEventData
{
    virtual void accept(EventDataVisitor& visitor){ visitor.visit(*this); }
};

class MyKeyboardEventHandler : public EventDataVisitor
{
    void visit(KeyboardEventData& event)
    {
        // Do my stuff
    }
};

void main()
{
    // Poll for next event in the queue
    DisplayEvent ev;
    display->PollEvent(ev); // Fills ev with the needed data

    MyKeyboardEventHandler myHandler;
    ev.GetEventData().accept(myHandler);
}
share|improve this answer

Is casting a base class object to a derived class object given a type flag a good programming design?

No. Usually it is a symptom of your class hierarchy not being specialized enough (not covering the functionality it should through virtual functions), or your client code being too generic (i.e. your code working on base pointers or refs, when it should be working on a specialized type).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.