Solving game state with polymorphism

Question

I've been working on a small game as a hobby, and I've been having trouble finding a good way to control the state of the game that could scale easily. I couldn't find a solution that I felt was correct, so I wrote my own.

I created an abstract base class State that has an abstract function update that takes and returns a pointer to the State base class. All children of State's update function should check if conditions are met to change the State if so it should delete the current state, and return another child of State. If it shouldn't transition states it update its data members and return the current state for the next pass in the loop.

I'm not a very experienced C++ programmer, and I feel I'm in over my head. I came to this solution by myself, and even though it work correctly and appears like it will scale fine in the future I'm worried if I went about this wrong or if there are safer or more efficient ways to deal with this problem. Looking back at my code I'm surprised I can delete a pointer of a class inside one of its own functions. Also the update method by design is completely unsafe which could be trouble.

I cut the state machine out of my project, and made a simple model of how it could be used, with two example states.

main.cpp

#include "State.h"
#include "State1.h"
int main() {
    State* state = new State1();
    //Main loop, how state updates itself.
    while (state->isRunning()) {
        state = state->update(state);
    }
    return 0;
}

State.h

class State {
public:
    State();
    virtual ~State();

    //Very unsafe method to update data in the main loop and change state.
    //Up to programmer to not mess things up.
    virtual State* update(State* currentState) = 0;
    //getter for running
    bool isRunning() const;
protected:
    static bool running;
};

State has a static variable running to keep track of when it should quit the main loop, like when the user closes the window. I felt a static variable was fine here since there should only ever be one state at a time.

State.cpp

#include "State.h"
bool State::running = true;

State::State() {

}

bool State::isRunning() const{
    return running;
}

State::~State() {

}

State1.h

#include "State.h"
class State1 : public State {
public:
    State1();
    ~State1();
    State* update(State* currentState);
private:
    int dataMember;
    int* dataMemberThatNeedsToBeDeleted;
};

State1.cpp

Here State2 requires an int to be constructed.

One advantage I found with this model is that states can easily share data between each other when transitioning. For instance if I was building a fighting game and it was currently on a character select state it could share the characters the players chose with the fighting state and that state could construct the characters to fight in that scene, and when one player wins it can share data like k/d ratio and winner with the score state.

States also are responsible to delete all memory allocated.

#include "State1.h"
#include "State2.h"
#include <iostream>
State1::State1() : dataMember(50), dataMemberThatNeedsToBeDeleted(new int(30)) {
    std::cout << "creating state1!" << std::endl;
}

State *State1::update(State *currentState) {
    std::cout << "State1's first data member is: " << dataMember << std::endl;
    std::cout << "State1's other data member is: " << *dataMemberThatNeedsToBeDeleted << std::endl;
    std::cout << std::endl;
    delete currentState;
    return new State2(dataMember);
    //In the actual code it would use if/switch statements to control the flow
    //example if this state was talking to an inputManager:
    //         if (key.pressed = esc) {
    //             running =  false;
    //             return currentState;
    //         } else if (key.pressed = i) {
    //             delete currentState;
    //             return new InventoryState(playerCharacter);
    //         } else
    //             return currentState;
}

State1::~State1() {
    delete dataMemberThatNeedsToBeDeleted;
}

State2.h

#include "State.h"

class State2 : public State {
public:
    State2(int _dataMember);
    ~State2();

    State* update(State* currentState);
private:
    int dataMember;
};

State2.cpp

#include "State2.h"
#include "iostream"

State2::State2(int _dataMember) : dataMember(_dataMember) {
    std::cout << "creating state2!" << std::endl;
}

State *State2::update(State *currentState) {
    std::cout << "State2's data member is " << dataMember << std::endl;
    std::cout << std::endl;
    running = false;
    return currentState;
}

State2::~State2() {}

And I get the correct output of:

creating state1!
State1's first data member is: 50
State1's other data member is: 30

creating state2!
State2's data member is 50

Answer 1

I see some things that may help you improve your program.

Fix your includes

Case matters to a C++ compiler, so when you write #include <IOStream> it is not the same as the standard #include <iostream> even if your compiler and/or operating system happens to accept it.

Prefer references to pointers

The argument to update is a State * but I think it should instead be a State &. You don't really ever want a nullptr there, and the current code doesn't check for it.

Don't delete yourself via pointer

Within the update routine we have these two lines:

State *State1::update(State *currentState) {
    // some other code
    delete currentState;
    return new State2(dataMember);
}

However, the usage in main is this:

state = state->update(state);

The problem with this is that the delete above invalidates all member data, including dataMember which is then erroneously used on the following line. Instead you could use something like this:

State *retval = new State2(dataMember);
delete currentState;
return retval;

I don't really like that very much either, but at least it does not reference invalid memory.

Consider using some other abstraction

The use of a single global variable for State::running effectively limits the design to only having a single useful instance of State at one time. If that's acceptable (and it seems to be in this use) then it suggests to me that a class hierarchy may not really the most appropriate abstraction mechanism. Instead, it's likely that an array (or vector or list or whatever) of functions may work better. Within each function, any memory allocation or freeing could happen without the user of the state machine having to worry about it.

Answer 2

Here is what I see has your main problem:

State* state = new State1();         // line 1
                                     // line 2
while (state->isRunning()) {         // line 3
    state = state->update(state);    // line 4
}                                    // line 5

How is the memory allocated in line 1 going to get freed? Every new should be paired up with a delete, but there isn't a one in this case.

Line 4 complicates the situation. Presumably either the call to ->update() could return the original state object or a brand new object. However, if you return a brand new object, you've lost the reference to the original one, so you can't delete it.

auto_ptr<>

A solution (and perhaps the best solution) to this problem is to use auto_ptr<> to manage the life cycle of your state objects.

Here is an example:

/*
 * An example of using auto_ptr<> with a Source.
 *
 * See also: http://www.gotw.ca/publications/using_auto_ptr_effectively.htm
 *
 */

#include <iostream>

class Foo {
  public:
    int n, k;
    Foo(int n0) {
      std::cout << "--> Creating Foo " << n0 << "\n";
      this->n = n0;
      this->k = 0;
    }

    std::auto_ptr<Foo> next() {
      this->k++;
      std::cout << "n = " << this->n << " k = " << this->k << "\n";
      if (this->k % this->n == 0) {
        return std::auto_ptr<Foo>( new Foo(this->n + 1) );
      } else {
        return std::auto_ptr<Foo>(this);
      }
    }

    ~Foo() {
      std::cout << "<-- Destructing Foo " << n << "\n";
    }
};

int main() {
  std::cout << "Hello, world\n";
  std::auto_ptr<Foo> state = std::auto_ptr<Foo>(new Foo(2));
  for (int i = 0; i < 11; ++i) {
    state = state->next();
  }
}

Running the program produces this output:

Hello, world
--> Creating Foo 2
n = 2 k = 1
n = 2 k = 2
--> Creating Foo 3
<-- Destructing Foo 2
n = 3 k = 1
n = 3 k = 2
n = 3 k = 3
--> Creating Foo 4
<-- Destructing Foo 3
n = 4 k = 1
n = 4 k = 2
n = 4 k = 3
n = 4 k = 4
--> Creating Foo 5
<-- Destructing Foo 4
n = 5 k = 1
n = 5 k = 2
<-- Destructing Foo 5

Sometimes the ->next() method returns itself and sometimes it returns a new object. In either case, all allocated objects are properly reclaimed so you won't leak any memory.

There is more on the use of auto_ptr<> here:

http://www.gotw.ca/publications/using_auto_ptr_effectively.htm

The idea of returning an auto_ptr<> is covered in the section "Ownership, Sources, and Sinks".