State Design Pattern - Don't want delete this pointer in member class

Question

I am using the example below to implement a state design pattern.

https://sourcemaking.com/design_patterns/state/cpp/1

I don't wanna delete the *this pointer inside a member class because is not safe (I will call other member function after delete).

class ON: public State
{
  public:
    ON()
    {
        cout << "   ON-ctor ";
    };
    ~ON()
    {
        cout << "   dtor-ON\n";
    };
    void off(Machine *m);
};

class OFF: public State
{
  public:
    OFF()
    {
        cout << "   OFF-ctor ";
    };
    ~OFF()
    {
        cout << "   dtor-OFF\n";
    };
    void on(Machine *m)
    {
        cout << "   going from OFF to ON";
        m->setCurrent(new ON());
        delete this; // <<< This line looks suspect and unsafe
    }
};

---

void ON::off(Machine *m)
{
  cout << "   going from ON to OFF";
  m->setCurrent(new OFF());
  delete this; // <<< This line looks suspect and unsafe
}

Is there a better approach to implement the state design pattern? I thought about use Singletons, but I want avoid using singletons.

Best regards,

Answer 1

"... but I want avoid using singletons."

That's probably one of their rare valid use cases, since states should be stateless themselves (which doesn't mean they don't need an instance), and accessible concurrently regardless of the current state machine's context.
So that means you actually just need one instance of a state at a time.

The example shown at the mentioned website, also gives you an unnecessary performance hit, at the cost coming from new and delete whenever state changes, which could be avoided to have steady static instances for the states.

Actually you could consider to provide state instances as with the Flyweight Design Pattern, which essentially boils down to have Singleton state instances.

---

But well, it depends. UML state diagrams actually allow to have non-stateless states (as composite states with history attributes, or active states).

Have a look at my STTCL template library concept document, it explains some of the aspects, and design decisions I have used, to develop that template library, and how it can be used properly.
Be assured you I don't have any single delete this; in there ;-).

---

I'd say that example the website currently gives, is really badly designed and not generally recommendable¹.
Using delete this isn't appropriate, dangerous and not acceptable, as you mentioned.

While using Singleton's is, if you have their valid use case at hand.

---

¹⁾ _{Sadly enough to notice this, since I've been using it as a "reference" for many design-patterns related questions here.}

Answer 2

The design used in your link is:

• a state machine keeps track of the pointer to the current state.
• the current state is responsible to set the new state when a transition occcurs
• it does so by creating the new state and instructing the machine to change the pointer
• once this is done, the it comits suicide by deleteing itself.

As explained here, this can work if extra care is taken. Your code respects the prerequisites.

The alternative would be that the state machine deletes the current state in setCurrent(). But this is worse: as soon as the machine has deleted the object, this object no longer exists, so that when setCurrent() returns, you're de facto in the same (dangerous) situation as before (with delete this), with the important difference that this would not be obvious.

Edit:

If you feel unconfortable with this construct, you could opt for a variant:

class Machine
{
   class State *current;     // current state
   class State *nextstate;   // null, unless a state transition was requested
   void check_transition();  // organise switch to nextstate if transition is needed 
public:
    Machine();
    void setCurrent(State *s)
    {
        nextstate = s;  // only sets the info about nextstate 
    }
    void set_on();
    void set_off();
}; 

In this scenario, the state-machine functions need to be slightly updated:

void Machine::set_on()
{
   current->set_on(this);  // as before
   check_transition();     // organise the transition if the transition was requested
}

The state transition would then be managed by the state machine:

void Machine::check_transition() { 
       if (nextstate) {
           swap (current, nextstate);
           delete nextstate;  // this contains the former current  
           nextstate = nullptr; 
       }
   }

The machine organises the deletion of the unused states. Note that this approach would allow the use of shared_ptr instead of raw pointers.

Here a small online proof of concept.

By the way: the machine destructor should also delete the current and next state, in order to avoid leaking. And in any case the state destructor should be defined virtual