Designing yet another a state machine

Question

I wanted to design a state machine with a few goals in mind:

• Encapsulate state logic within individual classes to keep them decoupled. (I'm very curious what the community thinks about this in particular.)
• Support multithreading, allowing a thread to fire up a stateful but blocking series of steps and watch them progress.
• Catch exceptions that occur within, transition to an error state, and eventually exit the state machine gracefully.
• Use the C++11 standard.

How would you improve this? Do my design goals sound reasonable?

#include <mutex>
#include <thread>
#include <unistd.h>
#include <iostream>
#include "Immutable.h"

struct NotImplementedException : public std::logic_error {
    NotImplementedException () : std::logic_error("Function not yet implemented.") {}
};

struct StateName : ImmutableString<StateName> {
    StateName(const std::string & name) : ImmutableString(name) {} 
};

template <typename T>
struct Callable {
    virtual T run(void) = 0;
};

struct State;
typedef std::shared_ptr<State> NextState;

class State : Callable<NextState> {

    const StateName _name; 

public:
    State(StateName name) : _name(name) {}
    State(const std::string & name) : _name(name) {}

    virtual NextState run(void) {
       throw NotImplementedException(); 
    };

    virtual bool isTerminalState() const {
        return false;
    }

    StateName getName() const {
        return _name;
    }
};

struct EndState : public State {
    EndState() : State("EndState") {}
    bool isTerminalState() const { return true; }
};

struct BarState : public State {
    BarState() : State("BarState") {}
    NextState run() {
        std::cout << "Now in " << getName() << std::endl;
        sleep(2);
        throw std::runtime_error("oopsie!");
        return NextState(new EndState());
    }
};

struct FooState : public State {
    FooState() : State("FooState") {}
    NextState run() {
        std::cout << "Now in " << getName() << std::endl;
        sleep(2);
        std::cout << "Going to BarState" << std::endl;
        return NextState(new BarState());
    }
};

struct StartState : public State {
    StartState() : State("StartState") {}
    NextState run() {
        std::cout << "Now in " << getName() << std::endl;
        sleep(2);
        std::cout << "Going to FooState" << std::endl;
        return NextState(new FooState());
    }
};

struct AbortState : public State {
    AbortState() : State("AbortState") {}
    bool isTerminalState() const { 
        return true;
    }
};

class StateManager : Callable<void> {

    NextState _current_state;
    std::mutex _mutex;

public:
    StateManager(State * start_state) : _current_state(start_state), _mutex() {}

    std::string getStateName() {
        std::lock_guard<std::mutex> lock(_mutex);
        return std::string(_current_state->getName().getValue());
    }

    bool isRunning() {
        std::lock_guard<std::mutex> lock(_mutex);
        return !_current_state->isTerminalState();
    }

    void setCurrentState(NextState next) {
        std::lock_guard<std::mutex> lock(_mutex);
        _current_state = next;
    }

    void run(void) {
        while(isRunning()) {
            try {
                auto next_state = _current_state->run();
                setCurrentState(next_state);
            } catch (...) {
                std::cout << "Caught an exception in " << _current_state->getName() << std::endl;
                std::cout << "StateManager terminating" << std::endl;
                setCurrentState(NextState(new AbortState()));
                return;
            }
        }
    }
};

int main() {
    StateManager state_manager(new StartState);
    std::thread thread(&StateManager::run, &state_manager);
    while(state_manager.isRunning()) {
        std::cout << state_manager.getStateName() << std::endl;
        usleep(250000);
    }
    thread.join();
}

---

Here's Immutable.h:

#ifndef LCC_IMMUTABLE_H
#define LCC_IMMUTABLE_H

template<typename T>
class AbstractImmutable {
    private:
        const T _value;
    public:
        AbstractImmutable(const T & value) : _value(value) {}
        const T & getValue() const { return _value; }

        inline friend bool operator<(const AbstractImmutable<T> & a, const AbstractImmutable<T> & b) {
            return a.getValue() < b.getValue();
        }

        inline friend bool operator>(const AbstractImmutable<T> & a, const AbstractImmutable<T> & b) {
            return a.getValue() > b.getValue();
        }

        inline friend bool operator==(const AbstractImmutable<T> & a, const AbstractImmutable<T> & b) {
            return a.getValue() == b.getValue();
        }

        const AbstractImmutable<T> operator+(const AbstractImmutable<T> & that) const {
            return AbstractImmutable<T>(getValue() + that.getValue());
        }

        const AbstractImmutable<T> operator-(const AbstractImmutable<T> & that) const {
            return AbstractImmutable<T>(getValue() - that.getValue());
        }

        const AbstractImmutable<T> operator/(const AbstractImmutable<T> & that) const {
            return AbstractImmutable<T>(getValue() / that.getValue());
        }

        const AbstractImmutable<T> operator*(const AbstractImmutable<T> & that) const {
            return AbstractImmutable<T>(getValue() * that.getValue());
        }

        inline friend std::ostream & operator<<(std::ostream & out, const AbstractImmutable<T> & that) {
            out << that.getValue();
            return out;
        }
};

template<typename T>
class ImmutableString : public AbstractImmutable<std::string> {
    using AbstractImmutable::AbstractImmutable;
};

#endif

Answer 1

AbstractImmutable doesn't seem to do anything except provide operators to the underlying type. However, many of the operators aren't necessarily relevant (e.g. dividing a std::string).

When adding another ImmutableFoo object to the hierarchy (e.g. an ImmutableUInt), would we add all the operators to the base class? Why support addition, subtraction, multiplication, and division, but not the remainder, or bitwise operators? Note that none of these operators will make sense for std::string either.

There seems no reason to prefer the StateName : ImmutableString : AbstractImmutable<T> hierarchy over a simple (const) std::string member in State.

---

What is the purpose of Callable? Using State as a base class seems sufficient - there is no reason for Callable to exist in the provided code, since it isn't used.

(Note that Callable<NextState> is a different type from Callable<void>, as used by the StateManager. So they can't be stored by a common base class (if that's the intent), because there isn't one.)

---

State::run should probably be an abstract function (virtual NextState run() = 0;), to force derived classes to implement it and remove the need for the NotImplementedException.

isTerminalState() should probably also be abstract to prevent mistakes (forgetting to override it would be quite easy).

Note that AbortState would throw a NotImplementedException with a somewhat misleading message if run. "Function must not be called" would be more accurate.

---

We should use the override keyword to indicate overriding virtual functions in derived classes (if not also the virtual keyword).

---

Use std::this_thread::sleep_for() as a cross-platform solution, instead of sleep or usleep.

---

Note that it's possible to create a std::shared_ptr directly from a std::unique_ptr. StateManager could therefore take the starting state by unique_ptr in the constructor (enforcing transfer of ownership) rather than the ambiguous raw pointer.

State::run() should probably also return a std::unique_ptr.

... In fact, is there a reason to use std::shared_ptr here at all?

---

I believe it is generally preferred to make mutex data-members mutable, allowing for const-correctness (e.g. for StateManager::isRunning() and StateManager::getStateName()).

---

Should StateManager::setCurrentState() really be public?

---

We should use std::exception_ptr to propagate an exception to the main thread. This removes the need for the AbortState.

---

Instead of (presumably) looping in the EndState until the main thread checks on us, why not return an empty NextState pointer in State::run? If we check for this and return in the StateManager, then we don't need the EndState or the isTerminalState() functions either.

---

Some other things to maybe try:

• It might be useful to keep track of whether a given state is currently-running, has-been-created-and-is-waiting-to-be-run, or has-ended-and-is-waiting-to-be-destroyed.

  This system is perhaps somewhat contrary to C++ principles of RAII. Perhaps it could be re-worked so that State objects exist only while they are being run. This could be done by changing the State::run() to return a factory-function used to create the next state, rather than the created state itself.

• Perhaps the State class-hierarchy could be entirely eliminated in favor of something like:

  struct State {
      std::string _name;
      std::function<std::unique_ptr<State>()> _function;
  };