Instances with global scope in a class

[fstigre]

Hi,

Silly question, I thought I knew about this already until last night when I tried to do something and realized that I wasn’t sure about the proper way to do it.

What is the proper way to insatiate an instance that will be used in multiple functions?

In other words what would be the proper way to instantiate and instance if for instance, I have ClassA and ClassB and I want to use an instance of ClassB in multiple functions/methods in ClassA.

This is how I would do it…

ClassA.h

#ifndef MAINWINDOW_H
#define MAINWINDOW_H

#include “ClassB”
…
private:
ClassB *myInstanceB = new ClassB;// is this bad practice?
...
#endif // MAINWINDOW_H 

.cpp

#include “ClassB”

ClassA::ClassA()
{
}
void ClassA::functionOne()
{
 myInstanceB->doSomething();
}
void ClassA::functionTwo()
{
 myInstanceB->doSomethingElse();
}

Is this bad practice, should I be using getters and setter or what is the proper way to do this?

[kevinkjt2000]

You should only be instantiating the private variable in ClassA's constructor. Unless, you want a static variable? A static variable would mean that all instantiations of ClassA would share the same variable.

[fstigre]

First of all thank you for your reply.

If I declare it in ClassA's constructor I get an error when I try to use it in another method.

#include “ClassB”

ClassA::ClassA()
{
  ClassB *myInstanceB = new ClassB;
}
void ClassA::functionOne()
{
 myInstanceB->doSomething();
}
void ClassA::functionTwo()
{
 myInstanceB->doSomethingElse();
}

// error: myInstanceB was not declared in this scope

[MrHutch]

// Header
class ClassA
{
   ClassB *myInstanceB;
};

// Source
ClassA::ClassA()
{
   // Edited as per the Disch review. :-P
   myInstanceB = new ClassB();
}

ClassA::~ClassA()
{
   if( myInstanceB )
   {
      delete myInstanceB;
      myInstanceB = NULL;
   }
}

[fstigre]

Thank you iHutch105.

For some reason I thought I tried this before and I got an error but I must have been doing something wrong.

Cool, is this considered lazy instantiation?

What would happen if I don't delete it in the deconstructor, would that be considered a memory leak?

ClassA::~ClassA()
{
   if( myInstanceB )
   {
      delete myInstanceB;
      myInstanceB = NULL;
   }
}

Thanks

[Disch]

ClassA::ClassA()
{
   if( !myInstanceB )  // This is probably overkill. Old habits...
      myInstanceB = new ClassB();
}

That's not just overkill.. it's actually flat out incorrect =P

myInstanceB has not been initialized at that point, so there is no way to guarantee it will be null.

More often than not, the above ctor will not allocate anything and will leave myInstaceB as a bad pointer.

EDIT:

This is arguably overkill

ClassA::~ClassA()
{
   if( myInstanceB )
   {
      delete myInstanceB;
      myInstanceB = NULL;
   }
}

For 2 reasons:
1) delete does nothing if the pointer is null, so there's no reason to check for null first (delete already does it)
2) no reason to null the pointer afterwards because this is the dtor and myInstanceB will literally not exist anymore after this function completes.

ClassA::~ClassA
{
    delete myInstanceB;
}

This is really all you need.

fstigre wrote:

What would happen if I don't delete it in the deconstructor, would that be considered a memory leak?

Yes.

Every new that does not have a matching delete will leak memory. If you do not delete this in the dtor, then every time you instantiate a ClassA object it will leak a ClassB object.


Furthermore... if you have a pointer in a class... the compiler provided copy constructor and assignment operators will not work and will cause your program to break if you try to use them.

So if you do this, you really should do one of the following:


1) Write your own copy ctor / assignment operator:

class ClassA
{
public:
    ClassA(const ClassA& rhs)
    {
        myInstanceB = new ClassB( *rhs.myInstanceB );
        // ... copy any and all other members here
    }

    ClassA& operator = (const ClassA& rhs)
    {
        *myInstanceB = *rhs.myInstanceB;
        // ... copy and and all other members here
        return *this;
    }
//... 

OR 2) forbid copying by deleting the copy ctor / assignment operator (using the = delete syntax if your compiler supports it... or by making them private and not giving them a body if your compiler doesn't)

[MrHutch]

fstigre wrote:

Cool, is this considered lazy instantiation?

Not as I understand it. See: http://en.wikipedia.org/wiki/Lazy_initialization#C.2B.2B

fstrigre wrote:

What would happen if I don't delete it in the deconstructor, would that be considered a memory leak?

Providing that instance isn't being pointed to by something else, yes.

[fstigre]

Thank you all for your help, that helps a lot.

[MikeyBoy]

Furthermore... if you have a pointer in a class... the compiler provided copy constructor and assignment operators will not work and will cause your program to break if you try to use them.

To go into a bit more detail:

The copy constructor and assignment operators will work in the sense that they will copy the value of the pointer. So the pointers in both the original object and the copy of the class will have the same value; that is, they will be pointing to the same area of memory.

With the class the OP has written, this will cause problems, because the code appears to assume that ClassA has complete ownership of the ClassB object it creates. However, if you have two classes both trying to own the same bit of memory you're going to have problems.

Specifically, you'll have a problem when objects are destroyed. When one of the ClassA objects is destroyed, it will delete the memory for its ClassB object. However, the second ClassA object is still pointing to the memory that's been freed. If it tries to use it, it will cause problems because that memory no longer contains a valid ClassB object. And when you try and destroy the second ClassA object, it will attempt to delete memory that's already been deleted, which will cause undefined behaviour.