Designing a state machine in C++ using the STL

I have this working code for C++ state machine using STL (not using boost state chart).

The purpose of posting is two-fold:

-
To share a simple and working code C++ state machine using the STL for the sparse 'state' and 'event' function matrix.

-
To seek enhancements of implementation, since with this approach I have to init state machine for every new object created, which is not necessary since state-event function are same for all objects of a given class.

(Note: while compiling using g++, use the "-std=c++0x" option and link both .cpp files to get the executable)

File name: BasicFsmT.h

```
//////////////////////////////////////////////////////////////////////
// File name : BasicFsmT.h
// Purpose : To define simple FSM implementation base class.
// This class to be inherited
// by any class which would like to implement FSM
///////////////////////////////////////////////////////////////////////
#ifndef _BasicFsmT_h
#define _BasicFsmT_h

#include
#include
#include
#include

using namespace std;

class BasicFsm
{
public :
BasicFsm(); // Default Constructor
BasicFsm(string initialState);
virtual ~BasicFsm();

// StateMachine Method prototype
typedef int (pFsmMethodT) (BasicFsm pFSM, string event, int info);

virtual int AddState(string state);
virtual int AddStateEventMethod(string state, string event, pFsmMethodT);
virtual string GetState();

virtual int Fsm(string event, int info);

virtual int TransitionToState(string state);
virtual int SetPreviousEvent(string event);

protected :

vector m_statesVec; // all valid states list in the fsm
vector m_eventsVec; // all valid events list in the fsm
string m_fsmStateStr; // current state of the state fsm
string m_fsmPrevEventStr; // the last event procesed by the fsm
string m_fsmPrevStateStr; // the previous state of fsm

// the key is state and event strings concatanated together
map m_stateEven

Two beginners mistakes:

Leading underscore in identifers

#ifndef _BasicFsmT_h
#define _BasicFsmT_h

Using a leading underscore is a bad idea. The exact rules are complex and people usually get them wrong. So just don't use them and you will be safe.

See: What are the rules about using an underscore in a C++ identifier?

using directives.

using namespace std;

Never do this (at file scope) in a header file (any using directive). It means that if I include your code all the standard libraries have been imported into the global namespace for my code which may break it. This may potentially stop people from using your code.

Even in source files this is a bad idea. The whole reason the namespace is called std rather than standard is so prefixing items with std:: is not a major problem. Please start to use this.

If you really don't want to use std:: (you should) then you import specific things by doing

using std::cin;  // only imports std::cin into the current namespace.

This is also usable within a scope:

void foo()
{
    using std::cin;
    cin << "Hi there\n";
}
void bar()
{
    cin << "This will fail to compile\n";
}

You shoudl read this: Why is 'using namespace std;' considered a bad practice in C++?

The most important answer for me is: https://stackoverflow.com/a/1453605/14065

Interfaces:

Your BasicFsm uses function pointers. This is very C. An easy way is to define an interface that implements the method. This will make the code easier to read and write:

// StateMachine Method prototype
typedef int (*pFsmMethodT) (BasicFsm* pFSM, string event, int info);

// I would write:
struct  FsmMethodInterface
{
    virtual ~FsmMethodInterface() {}
    virtual int doWork(BasicFsm& pFSM, std::string event, int info) = 0;
};
// Note: unless pFSM can be NULL (I suspect it can't) then you should
//       pass it by reference (then you don't need to check for NULL).

This is also more useful as it allows you to maintain state in the callback if required.

Constructor

/// Description : Default Constructor, called by derrived classes object
/// @param[in] 
/// @returns 
BasicFsm::BasicFsm()
{
    m_statesVec.clear();
    m_eventsVec.clear();
    m_fsmStateStr.clear();
    m_fsmPrevEventStr.clear();
    m_fsmPrevStateStr.clear();
    m_stateEventToMethodMap.clear();
}

This is doing no useful work. All those members are constructed empty. Just remove the constructor if it does nothing explicit.

Comments:

Comments that do nothing but repeat what the code says are worse than useless. They can fall out of sync with the code and provide no meaningful information to the a maintainer (and thus waste space).

/// Description : Add a state to be handled by the fsm
///     
/// @param[in] : state to be handled by the fsm
/// @returns 
int BasicFsm::AddState(string state)
{

The code explains what it is doing (with menaingful method/variable names). The comments should explain WHY it is doing it. If the why is obvious that don't write anything.

Exceptions:

} // end try
catch(exception& Error) {
    stringstream logErr;
    logErr << "Exception at BasicFsm::fsm, Error : " << Error.what();
}

Best to catch exceptions by const reference. YOu probably don't want to alter the exception.

Also exceptions do not need to be derived from std::exception (they could be std::string or int or anything else. catch(...) catches anything. But if you use this the best you can usually do is log and re-throw the exception.

So you logged the exception (which is good). But is that enough (It may be I don;t the code well enough). If an exception was thrown can you continue? The fact that something failed would suggest that unless you explicitly fix it the state of the machine is not good. If you cant't fix it then after you log the exception you should probably re-throw it.

Below is a discussion Seth and I are having.

It turns out Seths comments are more accurate then mine.

I am leaving the original comments for context.

In modern C++ functors and duck typing are more heavily used (then would be suggested by my answer above).

Seth thinks that they can be used in the current situation (I hope they can). But I can see how this will be done. We needed more space than the comments provided so we are working in the answer at the moment. This is an ongoing discussion which when resolved will be cleaned up.

From @Seth

So the more modern way to add generic functions would be:

int AddStateEventMethod(string state, string event, std::function func)
{
    /* CODE AS BEFORE */

    // Add the function pointer to be called for the specified state-event pair

    stateNEvent = state + event;
    m_stateEventToMethodMap[stateNEvent] = func;
}

std::map> m_stateEventToMethodMap;

Response from @Loki

Unfortunately this provides exactly zero benefit over the original and as such is not really modern C++. The whole point of modern C++ duck typing is that you coul

StackExchange Code Review Q#20273, answer score: 10