2
\$\begingroup\$

Is everything alright with this code? What can I add to it?

state.h:

#pragma once

class State
{
public:
    virtual void update(float dt) = 0;

    virtual void draw(float dt) = 0;
};

state_machine.h:

#pragma once
#include <memory>
#include <stack>
#include "state.h"

using StateRef = std::shared_ptr<State>;

class StateMachine
{
public:
    void push_state(const StateRef& state);

    void push_state(State* state);

    template<class T>
    void push_state();

    StateRef pop_state();

    StateRef get_current_state() const;

    void update(float dt);
private:
    std::stack<StateRef> stack;
};

template<class T>
inline void StateMachine::push_state()
{
    push_state(StateRef(new T));
}

state_machine.cpp:

#include "state_machine.h"
#include "state.h"

void StateMachine::push_state(const StateRef& state)
{
    stack.emplace(state);
}

void StateMachine::push_state(State* state)
{
    push_state(StateRef(state));
}

StateRef StateMachine::pop_state()
{
    auto state = get_current_state();

    stack.pop();

    return state;
}

StateRef StateMachine::get_current_state() const
{
    return stack.top();
}

void StateMachine::update(float dt)
{
    if(!stack.empty())
    {
        auto state = get_current_state();

        state->update(dt);
        state->draw(dt);
    }
}

Dummy usage example:

#include <sm/state_machine.h>
#include <iostream>

struct Foo: public State
{
    void update(float dt) override
    {
        std::cout << "(updated)" << std::endl;
    }

    void draw(float dt) override
    {
        std::cout << "(drawed)" << std::endl;
    }
};

int main()
{
    StateMachine machine;

    machine.push_state<Foo>();

    machine.update(0.0f);

    machine.pop_state();

    machine.update(0.0f);

    return 0;
}

GitHub repo

How can I add and use "Controller" component? (component that handles input and etc.). Is it necessary? (MVC pattern)

\$\endgroup\$
  • \$\begingroup\$ Oh, well StateMachine::get_current_state() must throw exception when stack is empty. I forgot about it \$\endgroup\$ – nuke_bird Nov 6 '17 at 13:06
  • 1
    \$\begingroup\$ you can add it. Unlike SO, on CR receiving an answer might take quite a while. \$\endgroup\$ – Incomputable Nov 6 '17 at 13:41
  • \$\begingroup\$ @Incomputable what is CR? \$\endgroup\$ – nuke_bird Nov 6 '17 at 15:54
  • \$\begingroup\$ Code review, this site. \$\endgroup\$ – Incomputable Nov 6 '17 at 15:56
  • \$\begingroup\$ Oh, for me it was not obvious :) Ok, got it \$\endgroup\$ – nuke_bird Nov 6 '17 at 15:58
4
\$\begingroup\$

Good stuff, I particularly like State being a purely abstract interface, that's exactly how to best do modern inheritance.

State needs a virtual destructor

This is 100% required in this case, since you delete states from pointers of State.

Reduce API surface

You have too many push_state() functions

void push_state(const StateRef& state);
void push_state(State* state);

template<class T>
void push_state();

You would be better off having just the single:

void push_state(const StateRef& state);

What looks like "convenience" to you just ends up being "confusing" for possible users of the class as they have to wonder "which one of these should I be using?"

Take objects by value if you are taking ownership

void push_state(const StateRef& state);

should be:

void push_state(StateRef state);

Your function currently will always go through a copy-constructor, and for shared_ptr<> this is definitely non-trivial as that has to be thread safe.

By taking the StateRef by value, this gives users control over whether to provide a RValue, move in a LValue, or provide a copy of some other object. Maximum flexibility without any overloads.

This brings us to:

Prefer unique_ptr over shared_ptr

I see no shared ownership scemantics here, unique_ptr would be better simply for performance reasons. But you may have other parts of the larger codebase that warrant it. That comment is just about the code I see in isolation.

That's pretty much all I've got here.

\$\endgroup\$
  • \$\begingroup\$ You're quite right, I must add a virtual destructor to State class Initially, I had only one function "push_state(const StateRef&)", but I notice that it can't be called in this way: push_state(new SomeState); OK, sounds logical. I'll pass StateRef by value shared_ptr is used to allow the user to store the state elsewhere (if it needed). I know about shared_ptr overhead By the way, can State interface be modified to handle input events? Or all must be done by State's childrens inside update method? P.S. Your overview is really helpful, thank you \$\endgroup\$ – nuke_bird Nov 6 '17 at 15:43
  • \$\begingroup\$ In modern C++, you should almost never see a call to new (expect for placement new in certain cases), push_state(std::make_shared<SomeState>()); is what you want. \$\endgroup\$ – Frank Nov 6 '17 at 15:47
  • \$\begingroup\$ As far as handling inputs, there are many, many ways to tackle that. So yes, there are many ways to design the interface of State to accomodate input management. But it depends on too many factors for me to just tell you "how" \$\endgroup\$ – Frank Nov 6 '17 at 15:49
  • \$\begingroup\$ Most likely, to handle I/O events I'll use an event system (so I don't need to update State class) \$\endgroup\$ – nuke_bird Nov 6 '17 at 15:53

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.