no member function named "add" for std::map about fsm-cpp

[Lecrapouille]

@ltinphan @shamim-akhtar This is not the only bug :( ! For this particular point: there is a mixture with the FiniteStateMachine::add that Shamim probably wanted to call. Since the method is never called and C++ lazy, this code compiles file. A quick fix is mStates[stateID] = state;

But there are plenty of major issues:

• Please do not use 0 (== NULL from C) but use instead nullptr (that use type checking by the C++ compiler).
• State<T>* getState(T stateID) the presence of stateID should be checked, else the mStates[stateID] will create a new state before returning it (that we do not want).
• Preferred to use references than raw pointers (+ constness) State<T>* getCurrentState(), void setCurrentState(State<T>* state) => void setCurrentState(State<T> const& state).
• virtual State::~State() = default is missing and needed because of virtual methods + delete called. A real compiler will warn you.
• Why calling new and delete in the main ? 1/ Use std::unique_ptr instead ! 2/ If raw pointers are wanted, make the State machine release memory and call delete from its destructor.
• fsm->add(new TurnstileLockedState(*fsm)); this can be simplified by using 1/ template: fsm->add<TurnstileLockedState>() and 2/ let the add method pass this for you and let it call new (or better std::make_unique) for you. Do not let the user allocating memory for you !
• virtual ~Functor() {} this one is useless, you cannot override operator()
• class Functor is not used in the example and cannot be used anyway (and in the case it's possible, please update your blog to give an example) and Functor are in double with virtual methods such as virtual void enter() In deed, if derived classes from State does not override enter(), exit(), update() (which is the case in the example) methods from State will be called and in this case why having functors is not desired if (mOnEnter) (*mOnEnter)(*this); useless. Therefore, State shall have these methods do nothing ie virtual void enter() {} and let derived class override them.

I hope I did hurt you, but please fix all these issues: do not let your blog with these major bugs (the article is interesting anyway). You may use the design pattern State instead.

[shamim-akhtar]

Thanks for raising the issues. It's strange that I didn't check thoroughly after writing the quick tutorial and I have no excuse for that. I have pushed my amendments to a new branch. https://github.com/shamim-akhtar/fsm-cpp/tree/issues-Lecrapouille-ltinphan. I have yet to address the memory ownership issue. I am aware of the problems raised by @Lecrapouille and I have fixed some of this today morning. I will need to sit down and look thoroughly for the memory ownership, whether to use a shared_ptr for shared instances.

I have also added a simple test for the Functor. @ltinphan and @Lecrapouille please take a look at the changes and let me know your inputs.

Thanks again for your suggestions.

I will amend the tutorial after these fixes.

[Lecrapouille]

@shamim-akhtar no worries ! I made a quick code review. I still do not understand why functors are needed, I think you had them because you forget to make virtual ... override methods on derived classes (see my code review). You should give a try without Functor.

For the fsm->add(new TurnstileLockedState(*fsm)); method fsm->add(state_locked.get()); is not a good way to do it. I was thinking about something like:

template<class T>
T& FiniteStateMachine::add()
{
    mStates[id] = std::make_unique<T>(*this);
   return *mStates[id];
}

state_locked = fsm->add<TurnstileLockedState>();

With a check that T is a derived class from the State class: maybe using std::is_same ?

You do not need shared_ptr just use unique_ptr (but std::make_unique need C++14 or if C++11 is needed see https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique the Possible Implementation section) and return the content as references. It's fine to let mCurrentState as a raw pointer.

State<T>& getState(T stateID)
{
      return *mStates[stateID];
}

State<T>& getCurrentState()
{
      assert(mCurrentState != nullptr);
      return *mCurrentState;
}

Else in main.cpp class TurnstileUnLockedState:

  void update()
  {
    State<TurnstileStateType>::update();
    if (KeyPresses['p'])
    {
      printf(" - pushed, locking turnstile\n");
      mFsm.setCurrentState(TurnstileStateType::LOCKED);
    }
  }

you can add:

if (KeyPresses['p'])
{
    ...
}
else if (KeyPresses['c'])
{
      printf("Coin already inserted. Return the coin\n");
}

[shamim-akhtar]

@Lecrapouille Good suggestions. Thanks for the review. The idea of a functor is to allow the update, onEnter and onExit methods without deriving a new State class. I will look into this and implement the necessary. Thanks again for a detailed look into the code. Much appreciated.

[shamim-akhtar]

@Lecrapouille

if (KeyPresses['p'])
{
...
}
else if (KeyPresses['c'])
{
printf("Coin already inserted. Return the coin\n");
}

I wanted to show that event handling independent event handling is possible within the update of each state. That's the reason why I have two separate if statements in the individual states. This functionality will be apparent when we have a number of states.
I will be making these changes and making a pull request later today/

I have removed the functor. Yes, I guess it was redundant. It will be easier to just derive from State and implement those virtual functions viz., update, onEnter and onExit.

I think it was necessary to put deeper thought into the implementation. Thanks a lot for your time and your constructive feedback.