digint / tinyfsm

A simple C++ finite state machine library
https://digint.ch/tinyfsm
MIT License
951 stars 174 forks source link

transit<> shall be the last statement in react(), but isn't ... #11

Closed taliesin closed 6 years ago

taliesin commented 6 years ago

The documentation says:

  1. Implement Actions and Event Reactions

In most cases, event reactions consist of one or more of the following steps:

Change some local data
Send events to other state machines
Transit to different state

Important: Make sure that the transit<>() function call is the last command executed within a reaction function!

However in elevator.cpp:

class Moving
: public Elevator
{
  void react(FloorSensor const & e) override {
    cout << "Reached floor " << e.floor << endl;

    int floor_expected = current_floor + Motor::getDirection();
    if(floor_expected != e.floor)
    {
      cout << "Floor sensor defect (expected " << floor_expected << ", got " << e.floor << ")" << endl;
      transit<Panic>(CallMaintenance);
    }

    current_floor = e.floor;
    if(e.floor == dest_floor)
      transit<Idle>();
  };
};

If the expected floor is not the reported floor it transits to Panic, still sets the current floor to the floor sensor information and might even transit to Idle.

Yeah I know it's more or less cosmetic, but to avoid to confuse newbies (like me) I'd suggest:

class Moving
: public Elevator
{
  void react(FloorSensor const & e) override {
    int floor_expected = current_floor + Motor::getDirection();
    if(floor_expected != e.floor)
    {
      cout << "Floor sensor defect (expected " << floor_expected << ", got " << e.floor << ")" << endl;
      transit<Panic>(CallMaintenance);
    }
   else
   {
      cout << "Reached floor " << e.floor << endl;
      current_floor = e.floor;
      if(e.floor == dest_floor)
        transit<Idle>();
   }
  };
};
digint commented 6 years ago

Thanks, this was really confusing / plain wrong...

Fixed in: 4cc38dc1b15c02fbd0d2773171925497f6b7600b