Should objects delete themselves in C++?

Question

I've spent the last 4 years in C# so I'm interested in current best practices and common design patterns in C++. Consider the following partial example:

class World { public:     void Add(Object *object);     void Remove(Object *object);     void Update(); }  class Fire : Object { public:     virtual void Update()     {         if(age > burnTime)         {             world.Remove(this);             delete this;         }     } } 

Here we have a world responsible for managing a set of objects and updating them regularly. Fire is an an object that might be added to the world under many different circumstances but typically by another object already in the world. Fire is the only object that knows when it has burned out so currently I have it deleting itself. The object that created the fire is likely no longer in existence or relevant.

Is this a sensible thing to do or is there a better design that would help clean up these objects?

Answer 1

You have made Update a virtual function, suggesting that derived classes may override the implementation of Update. This introduces two big risks.

1.) An overridden implementation may remember to do a World.Remove, but may forget the delete this. The memory is leaked.

2.) The overridden implementation calls the base-class Update, which does a delete this, but then proceeds with more work, but with an invalid this-pointer.

Consider this example:

class WizardsFire: public Fire { public:     virtual void Update()     {         Fire::Update();  // May delete the this-object!         RegenerateMana(this.ManaCost); // this is now invaild!  GPF!     } } 

Answer 2

The problem with this is that you're really creating an implicit coupling between the object and the World class.

If I try to call Update() outside the World class, what happens? I might end up with the object being deleted, and I don't know why. It seems the responsibilities are badly mixed up. This is going to cause problems the moment you use the Fire class in a new situation you hadn't thought of when you wrote this code. What happens if the object should be deleted from more than one place? Perhaps it should be removed both from the world, the current map, and the player's inventory? Your Update function will remove it from the world, and then delete the object, and the next time the map or the inventory tries to access the object, Bad Things Happen.

In general, I'd say it is very unintuitive for an Update() function to delete the object it is updating. I'd also say it's unintuitive for an object to delete itself. The object should more likely have some kind of way to fire an event saying that it has finished burning, and anyone interested can now act on that. For example by removing it from the world. For deleting it, think in terms of ownership.

Who owns the object? The world? That means the world alone gets to decide when the object dies. That's fine as long as the world's reference to the object is going to outlast an other references to it. Do you think the object own itself? What does that even mean? The object should be deleted when the object no longer exists? Doesn't make sense.

But if there is no clearly defined single owner, implement shared ownership, for example using a smart pointer implementing reference counting, such as boost::shared_ptr

But having a member function on the object itself, which is hardcoded to remove the object from one specific list, whether or not it exists there, and whether or not it also exists in any other list, and also delete the object itself regardless of which references to it exist, is a bad idea.