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,

You almost never should `delete this;`?? What do you actually want to achieve (just the link doesn't count, make your quesiton self contained please). — πάντα ῥεῖ, Jun 09 '15 at 20:05
Regarding proper _State Design Pattern_ implementation, have a look at my small and nifty [STTCL](http://makulik.github.io/sttcl/) template library. — πάντα ῥεῖ, Jun 09 '15 at 20:08
I feel like trying to use delete like that really violates the spirit of RAII — coryknapp, Jun 09 '15 at 20:12
Why don't you use shared pointers? Have your main create the states, then let your state machine simply swap back and forth between the On and Off state. When the program is done, your main sets its shared pointer to null, and the shared pointer then gets rid of the state for you. — StarPilot, Jun 09 '15 at 20:50
@StarPilot What makes you think _shared pointers_ are really needed for this? — πάντα ῥεῖ, Jun 09 '15 at 22:13
He does not want to delete the *this* pointer and he does not want to use a singleton. Making it a shared pointer lets him drop his references safely. The ON and OFF instances will be deleted when there are no more references to them via a shared pointer. However, I think he should consider using singletons for this. The state machine does not need unique ON and OFF instances, just that a pointer/reference to the ON or OFF singleton. If there are 500 such state machines, it will not matter. They don't need to destroy the ON/OFF. Unless poster is storing more info in each ON/OFF state? — StarPilot, Jun 09 '15 at 22:22

score 3 · Answer 1 · edited May 23 '17 at 11:51

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

_"it does so by creating the new state and instructing the machine to change the pointer"_ That's just (performance) overhead, isn't it? The current state class needs to know the other state class changing to anyways? So why not simply use a _Singleton_ / _Flyweight_? It wouldn't add additional interdependency clutter over using this solution. — πάντα ῥεῖ, Jun 09 '15 at 22:17
I always wondered about the state pattern when states (who are subclasses) know about specific other states. It seems that we're spreading out bits of the state machine logic in those classes and that's harder to understand (and maintain). But, I'm a guy who likes to draw a state diagram first and make sure my design supports it. I've always liked the state transition logic being in a single class. I think it's more maintainable and the risk associated with deleting is reduced. — Fuhrmanator, Jun 11 '15 at 21:36
@Fuhrmanator I tend as well to prefer an isolated state transition logic. But it all depends on the context; as noted by GoF, the decentralised transition logic adds flexibility and easy extensibility even if it introduces additional undesired inter-state dependencies. — Christophe, Jun 11 '15 at 21:50

πάντα ῥεῖ · Answer 2 · 2015-06-09T22:50:42.013

"... 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.}

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

2 Answers2