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

delete this is very risky. THere's nothing "wrong" with it. It's legal. But there are so many things that can go wrong when you use it that it should be used sparingly.

Consider the case where someone has open pointers or references to the object, and then it goes and deletes itself.

In most cases, i think the object lifetime should be managed by the same object that creates it. It just makes it easier to know where destruction occurs.

Answer 2

It's not generally a good idea to do a "delete this" unless necessary or used in a very straightforward way. In your case it looks like the World can just delete the object when it is removed, unless there are other dependencies we're not seeing (in which case your delete call will cause errors since they aren't notified). Keeping ownership outside your object allows your object to be better encapsulated and more stable: the object won't become invalid at some point simply because it chose to delete itself.

Answer 3

That's a fairly common reference counting implementation and I've used that successfully before.

However, I've also seen it crash. I wish I could remember the circumstances for the crash. @abelenky is one place I have seen it crash.

It might have been where you further subclass Fire, but fail to create a virtual destructor (see below). When you don't have a virtual destructor, the Update() function will call ~Fire() instead of the appropriate destructor ~Flame().

class Fire : Object
{
public:
    virtual void Update()
    {
        if(age > burnTime)
        {
            world.Remove(this);
            delete this; //Make sure this is a virtual destructor.
        }
    }
};

class Flame: public Fire
{
private: 
    int somevariable;
};

Answer 4

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.

Answer 5

Others have mentioned problems with "delete this." In short, since "World" manages the creation of Fire, it should also manage its deletion.

One other problem is if the world ends with a fire still burning. If this is the only mechanism through which a fire can be destroyed, then you could end up with an orphan or garbage fire.

For the semantics you want, I would have an "active" or "alive" flag in your "Fire" class (or Object if applicable). Then the world would on occasion check its inventory of objects, and get rid of ones that are no longer active.

--

One more note is that your code as written has Fire privately inheriting from Object, since that is the default, even though it is much less common than public inheiritance. You should probably make the kind of inheritance explicit, and I suspect you really want public inheritiance.