Strategy Design Pattern in C++ - Potential Memory Leaks

Hi,

I was going thru Strategy design pattern and its possible that this doubt may be the outcome of my lack of complete understanding of its implementation in C++. Better to ask the experts. In the Composition class we generally have pointers to compositors which are used by Composition class to delegate tasks to use whichever strategy is appropriate at runtime. This helps in encapsulating the constantly changing part of the class design from constant part. However, pattern also specifies that these family of algorithms can be interchange dynamically which means we can create new strategies (compositor objects) and reassigned to already declared compositor pointer in composition class. We do this using "new" operator. Now since the pointer now points to new strategy object which was again created using "new" operator but somewhere else, how do we delete the old object which is in memory? If any of you have referred HeadFirst book on design pattern, they can use Duck example as source code for this. Otherwise sample code looks like below :

class Composition {
public:
       Compositor * comp;
        void setStrategy(Compositor * cp) {
               comp = cp;
        }
         
        void UseStrategy() {
               cout << " Using strategy.." << endl;
        }
        .
        .
        other methods...
};

class Derived : public Composition {
      Dereived() {
              comp = new Test1Compositor; // Imagine this is some subclass of Compositor
        }

       ~Derived() {
               delete comp;
        }
        ..... other methods
};

main() {
       Composition *c = new Derived;
       c->UseStrategy();
       Compositor *cp1 = new Test2Compositor;
       c->setStrategy(cp1);
       c->UseStrategy();
       delete cp1;
       delete c;
}

What happens to Test1Compositor object that was created?

Thanks.
James BondSoftware ProfessionalAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

jkrCommented:
>>What happens to Test1Compositor object that was created?

In the above, that object is deleted in the destructor of 'Derived':

class Derived : public Composition {
      Dereived() {
              comp = new Test1Compositor; // Imagine this is some subclass of Compositor
        }

       ~Derived() {
               delete comp; // 'Test1Compositor' is properly disposed here
        }
        ..... other methods
};

Open in new window


This destructor is invoked when the last line of your 'main()' is executed when 'c' is deleted.
0
James BondSoftware ProfessionalAuthor Commented:
ok. But in setStrategy method we are re-pointing comp to cp1 which is different object of different strategy (Test2Compositor) than Test1Compositor. So in destructor of Derived, what are we deleting? Whats get deleted in below line in main function? -

delete cp1;
0
jkrCommented:
That's correct. In order to take that into account, 'setStrategy()' should delete any object that's associated with the enclosing class (if there is one), e.g.

class Composition {
public:

       Composition() : comp(NULL) {} // default constructor, initialite pointer to NULL
       Compositor * comp;
        void setStrategy(Compositor * cp) {
               if (comp) delete comp; // take care of any previous instance
               comp = cp;
        }
         
        void UseStrategy() {
               cout << " Using strategy.." << endl;
        }
        .
        .
        other methods...
};

class Derived : public Composition {
      Dereived() {
              comp = new Test1Compositor; // Imagine this is some subclass of Compositor
        }

       ~Derived() {
               delete comp;
        }
};

Open in new window

0
Determine the Perfect Price for Your IT Services

Do you wonder if your IT business is truly profitable or if you should raise your prices? Learn how to calculate your overhead burden with our free interactive tool and use it to determine the right price for your IT services. Download your free eBook now!

James BondSoftware ProfessionalAuthor Commented:
ok. That will solve the problem. But it doesn't look good from design point of view. Allocating in constructor, deleting in some method in base class etc. The flow of heap allocation and de-allocation should be smoother else memory management may become headache when no. of objects increases and more threads come in to picture. Is there any better way of this implementation?
0
jkrCommented:
I'd srather use a reference to the Compositor object than a pointer, because that does not tie the ownership to the Composition object - either that or don't use 'new' but pass the addresses of instances.
0
James BondSoftware ProfessionalAuthor Commented:
How do i initialise the reference then? Cant make it null. Most likely will use initialization list. There has to be existing object. Where will it exist? Also who owns reference then? They say reference internally is implemented as pointer. So how ownership management happen in this case?
0
jkrCommented:
No, you can't make it NULL, biut providing a class member that constitutes a valid invalid reference (no pun intended) would make it.
0
James BondSoftware ProfessionalAuthor Commented:
Didnt understand that. Can you show some sample code?
0
sarabandeCommented:
you can't use a reference member because a reference member can't be changed later.

so, there is no alternative to the comp pointer member beside you would use the factory pattern to creating instances of the Compositor derived class. then, you could have the 'factory key'  as a member of your composition class.  that also would allow you to pass the 'factory key' to the setStrategy member function rather than a pointer. then the setStrategy function would delete the old member pointer and create the new one using the factory:

class Compositor
{
    // assume the 
    static std::map<std::string, CpCreateFunc> factory;
public:
    static bool addToFactory(const std::string & name, CpCreateFunc func)
    {
            factory[name] = func;
    }
    static Compositor * createByName(const std::string & name)
    {
          if (factory.find(name) == factory.end()) return NULL;
          return factory[name]();
    }
    ....

class Test1Compositor : public Compositor
{
     static bool infactory;
     static Compositor * create() { return new Test1Compositor };
     ... 


class Composition
{
     Compositor * comp;
public: 
     void setStrategy(const std::string & s)
     {
            // delete old strategy
            delete comp;
            comp = Compositor::createByName(s);
     }
     ....

// Test1Compositor.cpp
...
// static initialization
bool Test1Compositor::infactory = 
     Compositor::addToFactory("Test1Compositor", Test1Compositor:.create);

Open in new window


the advantage of the above is that you can reference your strategies by name and not by pointer, what would allow the Composition class to encapsulate the internal pointer.

moreover, the factory pattern would even allow to have a string (or integer) member rather than a pointer member cause you always can create a suitable pointer out of the factory key.

Sara
0
Subrat (C++ windows/Linux)Software EngineerCommented:
If my understanding is true about heap deletion issue, then I could suggest that why not to use smart_pointer to take care of memory allocation & deallocation?Like- std::unique_ptr or something similar.
0
Subrat (C++ windows/Linux)Software EngineerCommented:
0
James BondSoftware ProfessionalAuthor Commented:
Can you please show sample implementation using smart pointers? They generally have move semantics and i want to see how ownership affect memory management.
0
ambienceCommented:
       ~Derived() {
               delete comp;
        }

Open in new window

This will never get executed because Composition is missing a virtual destructor. This is one of the nuisances of C++ language that is often the source of resource leaks. Add the following to fix it

class Composition {
public:
       Compositor * comp;
        virtual ~Composition() { 
             delete comp;
[b]
             // NOT a good design to delete it in Derived, therefore I have deleted it here
             // You can now remove ~Derived() altogether[/b]
       }
};

Open in new window

I would also recommend to use smart pointers. For a start, all you have to do is change this

Composition* comp;

to

std::unique_ptr<Composition> comp; // may require including a header file

and remove all instances of delete comp.
0
ambienceCommented:
Just realized that you are deleting comp externally

       Composition *c = new Derived;
       c->UseStrategy();
       Compositor *cp1 = new Test2Compositor;
       c->setStrategy(cp1);
       c->UseStrategy();
       delete cp1;
       delete c;

You have to make an ownership decision, who own cp1? If Compositor is supposed to own it then let it delete it in destructor, otherwise dont delete it inside Compositor, not even in setStrategy.

It is often easy to decide, as most of the time theres only one logical owner of a resource. However, you can also use std::shared_ptr and share ownership. All you have to do is change

Composition* comp;

to

std::share_ptr<Composition> comp;

and

std::share_ptr<Composition> c = make_shared(new Derived);
c->UseStrategy();

        void setStrategy(const std::share_ptr<Composition>& cp) {
               comp = cp;
        }

        void setStrategy(std::share_ptr<Composition>&& cp) {
               comp = std::move(cp);
        }

Hope that helps ...
0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
ambienceCommented:
ops ... its supposed to be ... shared_ptr ... not ... share_ptr
0
James BondSoftware ProfessionalAuthor Commented:
I need to test this. Please give me some more time.
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
C++

From novice to tech pro — start learning today.