Search code examples
c++c++11class-design

Is deleting an object from that object's method or from another class constructor considered bad practice?


I have a class A whose objects store a pointer.

This class's objects will either be used in another class (B)'s constructor which will take ownership of that pointer (thus making the A object useless) or be told that their pointer is useless and that they aren't needed anymore (which means they will free the memory pointed to by the pointer they hold).

So these objects have a pointer and a free method which deletes that pointer.

My question is, since the objects won't be needed anymore after they're either used by B's constructor or their method free is used, is it considered bad design / practice to tell B's constructor to also delete the A object it used, and to insert a delete this instruction at the end of A::free()? Or should I stick to manually delete those?

A little example:

A* myobj = new A(new Value(12));
B* myfinalobj;

if (is_useful(myobj)) {
    myfinalobj = new B(myobj);
    delete myobj;
}
else {
    myobj->free();
    delete myobj;
}

VS

A* myobj = new A(new Value(12));
B* myfinalobj;

if (is_useful(myobj)) {
    myfinalobj = new B(myobj); //myobj deletion is done by B::B(A*)
}
else {
    myobj->free(); //myobj deletion is done internally by A::free()
}

Solution

  • In general you should allocate the objects "close" to where you deallocate them.

    So when you only use it like in your example you should keep the "delete" close and visible to the "new". There may be reasons where you are not able to do this and you have to comment this clearly when you take ownership.

    Same as when you allocate objects in a constructor, delete them in the same class etc.

    Howerver:

    Since you tagged this c++11. There are smart pointers who display your intention and help you doing this.

    If you only want one instance to have "ownership" you can use a std::unique_ptr<A> and move it into the the class B. It will then delete the instance of A automatically when B gets destructed.

    If you have shared ownership, you can use std::shared_ptr<A>.

    This will delete the instance of A when nobody has a reference to the pointer anymore.

    If you can prefer unique_ptr to shared_ptr though.