Finite State Machine in C++

Question

I made a state machine.

You can press 0 and 1 to switch between the hypothetical menu and playing state.

I hope you can help me improve it further.

StateManager.h

#pragma once

#ifndef StateManager_H
#define StateManager_H

#include <iostream>
#include "State.h"

/*
State classes
*/

class StateManager {
public:
    // Define m_current_state upon the creation of this class (since that prevents there being a nullptr error in change_state
    StateManager(State* state)
        : m_current_state(state)
    {}

    void change_state(State* state) {
        m_current_state->on_exit();
        m_current_state = state;
        m_current_state->on_enter();
    }

    void update_state(StateManager* manager) {
        m_current_state->on_update(manager);
    }

    State* get_state() {
        return m_current_state;
    }

private:
    State* m_current_state;
};

#endif

State.h

#pragma once

#ifndef State_H
#define State_H

#include "StateManager.h"

// I'm using forward declaration here since I've had a problem with circular dependency
class StateManager;

class State {
public:
    // All states have to have these three functions
    virtual void on_enter() = 0;
    virtual void on_update(StateManager* state_manager) = 0;
    virtual void on_exit() = 0;
};

#endif

IntroState.h

#pragma once

#ifndef IntroState_H
#define IntroState_H

#include "StateManager.h"
#include "State.h"
#include "GameState.h"

/*
Basic test state for demonstration
*/

class MenuState : public State {
public:
    MenuState();

    virtual void on_enter() override;

    virtual void on_update(StateManager* state_manager) override;

    virtual void on_exit() override;

private:
    bool should_change_state;
};

#endif

Introstate.cpp

#include "IntroState.h"

MenuState::MenuState()
    : should_change_state(false)
{}

void MenuState::on_enter(){
    std::cout << "Entering the menu state" << std::endl;
}

void MenuState::on_update(StateManager* state_manager){
    // update the menu
    std::cout << "Should the state be changed to the playing state" << std::endl;
    std::cin >> should_change_state;
    if (should_change_state == true) {
        state_manager->change_state(new PlayingState());
    }
}

void MenuState::on_exit(){
    std::cout << "Exiting the menu state" << std::endl;
}

GameState.h

#pragma once

#ifndef GameState_H
#define GameState_H

#include "IntroState.h"
#include "State.h"

/*
Basic test state for demonstration
*/

class PlayingState : public State {
public:
    PlayingState();

    virtual void on_enter() override;

    virtual void on_update(StateManager* state_manager) override;

    virtual void on_exit() override;

private:
    bool should_change_state;
};

#endif

GameState.cpp

#include "GameState.h"

PlayingState::PlayingState()
    : should_change_state(false)
{}

void PlayingState::on_enter() {
    std::cout << "Entering the playing state" << std::endl;
}

void PlayingState::on_update(StateManager* state_manager) {
    // update the menu
    std::cout << "Should the state be changed to the menu state" << std::endl;
    std::cin >> should_change_state;
    if (should_change_state == true) {
        state_manager->change_state(new MenuState());
    }
}

void PlayingState::on_exit() {
    std::cout << "Exiting the playing state" << std::endl;
}

Source.cpp

#include "StateManager.h"
#include "State.h"
#include "IntroState.h"

int main() {
    StateManager* manager = new StateManager(new MenuState());

    int exit = 0;
    while (exit != 1) {
        manager->update_state(manager);
    }

    return 0;
}

Thanks!

Answer 1

Last things first

Before I forget about it (because it's peripheral in your code), I'll point out the new StateManager(new MenuState());, which is among the don't of C++. Raw pointers are dangerous! if you don't delete the allocated memory afterwards, you'll leave a leak (and you didn't). So use smart pointers (std::unique_ptr or std::shared_ptr) instead. That's quite an interesting topic, but you'll have to study it for yourself. Or better still, allocate your objects on the stack (State_manager manager;)

The design

manager->update_state(manager);

is a bit weird, don't you think? What would it mean to write manager->update_state(other_manager)? Probably not much. So you could rewrite the update_state method like this:

void update_state() {
    m_current_state->on_update(this); // instead of current->on_update(removed_argument)
}

It's cleaner and completely equivalent. But then you have to write such things as:

while (exit != 1) {
    manager->update_state();
}

It isn't obvious. Where are the external input taken from? Following the thread, I see that the the state itself is responsible for managing the transition: it will ask user input and control if changing from the current state (i.e itself) to the next state (that it will have to determine too) is feasible. Without saying if it is a good design or not, I wonder whether it is useful in anyway to have a state manager at all. Its only role seems to call on_enter and on_exit, but baking that into the State class isn't difficult since on_enter and on_exit are part of its interface (or you could even dispense with it; just add instructions at the beginning and the end of the update function). It also keeps track of the current state, but a simple current_state variable would do the trick:

std::unique_ptr<State> current_state = std::make_unique<Intro_state>();
while (exit != 1) current_state->update();

But then you might also wonder why this exit is outside of the state machine. If there is an intro-state, a game-state

At that point I want to take a break and tell you that naming your file with a synonym or a paraphrases for the class it contains is a really bad idea. The Game_state class should be in Game_state.h, not in gaming_state.h, that doesn't make any sense.

then why isn't there an exit-state? It could do some clean-up, display credits, I don't know. So now we have:

std::unique_ptr<State> current_state = std::make_unique<Intro_state>();
current_state->update();

But then the current_state variable itself, and the allocation that goes along with it, are also superfluous. Intro_state().update(); would look weird, but what about intro();? So we have a function void intro() that asks if it needs to call void game() or void exit(), and a void game() that asks if it needs to call void intro() again or void exit(). The design pattern has disappeared, and yet we haven't removed any functionality or usability feature, only simplified. From that observation I feel I can conclude safely that there is a problem in your design.

My 3 cents

Many C++ manuals insist on the object-oriented part of the language and the so-called design patterns. It isn't, IMHO (which, that said, isn't really my own but one taken by most of the prominent figures of the C++ scene), a good way to learn C++. Try rather to explore the robustness of the language or its versatility.

Answer 2

only include headers you actually need in that file. No need to #include <iostream> in StateManager.h.

Leaks abound. You new every state but I don't see anywhere you delete them. Use smart pointers or have States clean themselves up on_exit. I prefer smart pointers though.

Calling change_state inside on_update will lead to surprising effects due to on_exit being called during that.

Instead return the next state:

void update_state(StateManager* manager) {
    unique_ptr<State> tmp = m_current_state->on_update(manager);
    if(tmp != nullptr)
        change_state(std::move(tmp));
}

Answer 3

On raw pointers

In my opinion, games are one of the places where raw pointers are okay. They're okay because you want to tightly control the creation and destruction of objects and generally avoid memory allocation during active play, so the functionality you gain from smart pointers is not particularly valuable and mostly clutters your code and slows things down in Debug builds. That said, if you're learning C++ you really do want to learn smart pointers; they're important and useful.

In any case, you are abusing your raw pointers because you're not carefully managing the lifetime of your objects. You must, must, must consider where and when an object will be deleted when you new it. You cannot afford to ignore this. And, in your case, I think you are doing it wrong:

You probably want to keep the states as permanent objects

Instead of doing new MenuState whenever you enter the menu state, you probably want to create the states once (storing them in an object which will manage their lifetime) and then pass a pointer to the existing new state to the StateManager when you change state. You could also consider hiding the pointers away and passing a suitable message for the new state instead.

You likely want to defer state changes

Instead of changing state the moment a state is changed, you probably want to defer the change until the appropriate point in your game update loop so that you can correctly synchronise sounds/graphics/etc for the menu. You also probably want to decouple exit and entry so that you can transition from one menu to the other and this means you will want the menu to update its exit over several frames before the next menu begins its entry over the next few frames.