Defining a certain member function

Question

Consider the following:

#include <iostream>

struct State { virtual ~State() = default; };

struct Drunk : State {
    void singWhileDrunk() {std::cout << "Singing while drunk.\n";}
};

struct Person {
    State* state;
    void singWhileDrunk() {dynamic_cast<Drunk*>(state)->singWhileDrunk();}  // Is this good?
};


int main() {
    Person bob;
    bob.state = new Drunk;
    bob.singWhileDrunk();
    // dynamic_cast<Drunk*>(bob.state)->singWhileDrunk();  // Using this is better?
}

What I wonder is if Person::singWhileDrunk() should really be defined in Person or not. singWhileDrunk() only has true meaning if the person is drunk, so to define it in Person seems wrong to me. However, it does simplify the code in main(), especially if it is to be used a lot.

(dynamic_cast<Drunk*>(bob.state)->singWhileDrunk();

is clearly more typing (and may run into difficulties if I want to redefine it everywhere it is used, e.g. change dynamic_cast to static_cast). Another issue I have is that in my program I have many different types of states, each with their own special functions, and to define them all in Person will really bloat the Person class with MANY, MANY functions that don't even seem to belong in Person. So there seems to be pros and cons to both choices and would like to hear what others have to say about this.

This is just an example of course. In reality, I have states like FlySpellState, with the function flies(), which also seems to have no place in Person (since people cannot fly normally), though it could be.

Answer 1

You should never really be doing any manual casting.
Virtual functions usually provide you with the functionality you are looking for.

How about:

struct State
{
    virtual void sing() = 0;
};

struct Drunk: State
{
    virtual void sing() { std::cout << "Singing while drunk.\n"}
}

struct Sober: State
{
    virtual void sing() { std::cout << "Singing Opera.\n"}
}

class Person
{
     std::auto_ptr<State>   state;
     public:
         Person()
            : state(new Sober)
         {}
         void sing()
         {
             state->sing();
         }
         void drink()
         {
             state.reset(new Drunk);
         }
         void sleep()
         {
             state.reset(new Sober);
         }
};

Then your code in main is even more trivial:

int main() {
    Person   bob;
    bob.drink();  // Don't allow manual changes in state.
                  // Provide actions that internally modify the state of your object.
    bob.sing(); 
    bob.sleep();
    bob.sing();
}

This is just an example of course. In reality, I have states like FlySpellState, with the function flies(), which also seems to have no place in Person (since people cannot fly normally), though it could be.

Sure. So what you want is a bunch of different states. These states can all be changed when diffent situations apply.

struct Action
{
    virtual void doAction()  {std::cout << "Nothing Happens\n";}
};

class Person
{

    std::map<std::string, std::unique_ptr<Action>>  actionMap;
    Action& getAction(std::string const& action)
    {
        static Action defaultAction;    // Will do Nothing by default.

        auto find = actionMap.find(action);
        return find == actionMap.end()
                  ? defaultAction
                  : *(find->second);
    }

    void sing()
    {
        getAction("sing").doAction();
    }
    void fly()
    {
        getAction("fly").doAction();  // Default do nothing unless you have read the scroll of flying
    }
    void drink()
    {
        actionMap["sing"].reset(new Drunk);
        actionMap["walk"].reset(new Stumble);
        actionMap["talk"].reset(new Slur);
    }
    void readScrollOfFlying()
    {
        actionMap["fly"].reset(new FlySpellState);
    }
};

Rather than using std::unique_ptr and lots of calls to new. You may want to look up the flyweight pattern.

Answer 2

To focus on your embedded question, the following code is a problem waiting to happen:

dynamic_cast<Drunk*>(state)->singWhileDrunk();

When state holds a pointer to a type that isn't Drunk or a subtype of Drunk, the dynamic_cast will return a nullptr. Thus this code is prone to undefined behavior and should be avoided. There are four main alternatives.

• If, as claimed in a comment, you know with certainty that state holds a pointer to Drunk, use static_cast<Drunk*> to indicate this knowledge and to avoid the overhead associated with a dynamic cast. However this is not substantiated by the code in the question, so I cannot suggest it here.
• As Loki Astari suggests in his answer, redesign the interface to use the dynamic nature of virtual methods. I would typically prefer this approach, as it is more common, and thus less likely to be misread or maintained incorrectly.

• Check the result of the dynamic_cast:

  Drunk* drunk = dynamic_cast<Drunk*>(state);
  if (drunk) drunk->singWhileDrunk();
  

• Use the non-pointer form of dynamic_cast which throws an exception instead. Make sure that you don't invoke undefined behavior if the original member was nullptr, though:

  if (state) dynamic_cast<Drunk&>(*state)->singWhileDrunk(); // note: may throw std::bad_cast
  

As a general comment, however, note that you don't have to make everything a method on Person. Sometimes it makes sense to create free functions that accept a Person& and extract state information. So assuming it makes sense to either directly access Person::state or to add a Person::getState(), you could write code like this:

void signIfDrunk(Person& person)
{
    Drunk* drunk = dynamic_cast<Drunk*>(person.state);
    if (drunk)
        drunk->singWhileDrunk();
}

It's hard to tell here which approach is better, but it's generally accepted that non-member non-friend functions are a better way to compose code.

Answer 3

You said,

Another issue I have is that in my program I have many different types of states, each with their own special functions, and to define them all in Person will really bloat the Person class with MANY, MANY functions that don't even seem to belong in Person. So there seems to be pros and cons to both choices and would like to hear what others have to say about this.

If you have to modify Person every time a new type of State is introduced to your program, it violates the Open/Closed Principle and is a symptom of sub-optimal design.

My suggestion will be to make State-specific functions external functions and not member functions of Person.

void singWhileDrunk(Person const& p)
{
   Drunk* drunk = dynamic_cast<Drunk*>(p.state);
   if ( drunk != nullptr )
   {
      drunk->singWhileDrunk();
   }
}