3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Can't a Person also sing while !Drunk? Seems to me that the state my alter the result of the function, but it's still the same member function call. \$\endgroup\$ – Edward Dec 9 '14 at 2:49
  • \$\begingroup\$ Well, that's the thing. There is Person::sing(), which truly belongs in Person, and then there is singWhileDrunk(), which seems to belong in Drunk only. And this is just an example. In reality, I have states like FlySpellState, with the function flies(), which clearly has no place in Person (since people cannot fly normally). \$\endgroup\$ – prestokeys Dec 9 '14 at 2:52
  • \$\begingroup\$ But the more I think about it, along with considering Edward's answer, I think I should place these functions in Person. I just worry about the "pollution" of many such functions in Person. \$\endgroup\$ – prestokeys Dec 9 '14 at 3:43
4
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ So bottom line: you also think it is alright to define fly() in Person even though a person normally never flies (even though fly() normally carries out defaultAction.doAction(),i.e. nothing). \$\endgroup\$ – prestokeys Dec 9 '14 at 19:09
  • \$\begingroup\$ @prestokeys: I don't have enough information to make that call. Maybe you can have a move(MovementType type) method. Where MovementType is an enum that has run, walk, swim, fly, .... \$\endgroup\$ – Martin York Dec 9 '14 at 20:01
  • \$\begingroup\$ Yes, you predicted correctly. I do have that function, but for LivingBeing, which is a parent class of Person. Person is also derived from Walk, Run, Climb, Swim, while Bird is derived from LivingBeing, Fly, Walk, Run. \$\endgroup\$ – prestokeys Dec 9 '14 at 20:55
  • \$\begingroup\$ @prestokeys: Prefer composition over inheritance as a general rule. \$\endgroup\$ – Martin York Dec 9 '14 at 22:32
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ No idea why the down-vote. I find your answer legitimate, so +1 to balance that ;) \$\endgroup\$ – glampert Dec 9 '14 at 17:04
  • \$\begingroup\$ But do I really need to check if (drunk != nullptr) when I'm certain that singWhileDrunk() is only called when the person is drunk? For example, singWhileDrunk() is called only after checking ahead of time if the person is drunk or not. \$\endgroup\$ – prestokeys Dec 9 '14 at 18:48
  • \$\begingroup\$ @prestokeys: If you are certain that your State* is a valid Drunk* you should use static_cast<Drunk*> instead to signal this to the reader (and compiler); dynamic_cast<Drunk*> signals that you don't know. Of course if it's not inherently obvious why the local code can know this, you may want to comment as well. \$\endgroup\$ – Michael Urman Dec 9 '14 at 19:37
  • \$\begingroup\$ @Michael. The only reason for dynamic_cast<Drunk*> is because I actually have a vector<State*> of concurrent states for Person (State being the base class), and there is virtual inheritance in State. \$\endgroup\$ – prestokeys Dec 9 '14 at 19:51
0
\$\begingroup\$

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();
   }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I also have proxy class PersonProxy derived from Person, that holds a Person* pointer actual. So instead of defining Person::singWhileDrunk() and then PersonProxy::singWhileDrunk() {actual->singWhileDrunk();} you suggest your solution together with void singWhileDrunk(PersonProxy& p) {singWhileDrunk(p->getActual());} ? In terms of violating the open-closed principle, isn't there equal amount of responsibility in adding the functions either way when new States are added? \$\endgroup\$ – prestokeys Dec 9 '14 at 20:43
  • \$\begingroup\$ @prestokeys, definitely. \$\endgroup\$ – R Sahu Dec 9 '14 at 20:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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