Copy constructor - assignment operator example

I need to do a deep copy with my copy constructor.  I've looked through a lot of the previous posts but I didn't see an thorough example that actually showed the content of the copy constructor or assignment operator.

Could someone look at the following code and comment if it is correct?

Many thanks.


class myClass
{
public:
      //constructor
      myClass(int* x);

      //copy constructor
      myClass(const myClass& copyMe);

      //assignment operator
      myClass& operator=(const myClass& rhs);

private:

      int* myX;
      int* myZ;
      string myString;
}


myClass::myClass(int* x)
{
      this->myX = x;

      //initialize private data members
      this->myZ = new int;
      *myZ = 0;
      this->myString = "";
}


myClass::myClass(const myClass& copyMe)
{
      myX = copyMe.myX;

      myZ = new int;
      myZ = copyMe.myZ;

      myString = copyMe.myString;
}

myClass& myClass::operator=(const myClass& rhs)
{
      if (this != &rhs){

            delete myX;
            delete myZ;

            myX = new int;
            myX = rhs.myX;

            myZ = new int;
            myZ = rhs.myZ;

            myString = rhs.myString;
      }
      return *this;
}
JohnSantaFeAsked:
Who is Participating?
 
DineshJolaniaCommented:
Yes.... I believe wayside has raised the valid point.
The code posted by you has some runtime issues as well. You need to modify the ctor, copy ctor and assign op function. The code should looks like as,

#include <string>
using namespace std;

class myClass
{
public:
      //constructor
      myClass(int* x);
      
      //copy constructor
      myClass(const myClass& copyMe);
      
      //assignment operator
      myClass& operator=(const myClass& rhs);
      
      ~myClass();

private:
      
      int* myX;
      int* myZ;
      string myString;
};


myClass::myClass(int* x)
{
      myX = new int;   //Need mem allocation.
      *myX = *x;  //Need to assign value instead of address, else runtime error on "delete myX"
      
      //initialize private data members
      myZ = new int;
      *myZ = 0;
      myString = "";
}

myClass::~myClass()
{
      delete myX;
      delete myZ;
}

myClass::myClass(const myClass& copyMe)
{
      myX = new int;
      *myX = *(copyMe.myX);  // Same here also
      
      myZ = new int;
      *myZ = *(copyMe.myZ);  // Same here also
      
      myString = copyMe.myString;
}
myClass& myClass::operator=(const myClass& rhs)
{
      if (this != &rhs){
            
            delete myX;
            delete myZ;
            
            myX = new int;
            *myX = *(rhs.myX);  // Same here also
            
            myZ = new int;
            *myZ = *(rhs.myZ); // Same here also
            
            myString = rhs.myString;
      }
      return *this;
}

int main ()
{
      
    int n1 = 23;
    int n2 = 42;
      
    myClass A(&n1);
      
    myClass B(&n2);
      
    B = A;
      
    return 0;
}

0
 
jkrCommented:
Looks good (except a missing ';') compiles fine and runs with no errors:

#include <string>
using namespace std;

class myClass
{
public:
     //constructor
     myClass(int* x);

     //copy constructor
     myClass(const myClass& copyMe);

     //assignment operator
     myClass& operator=(const myClass& rhs);

private:

     int* myX;
     int* myZ;
     string myString;
}; // ';' was missing


myClass::myClass(int* x)
{
     this->myX = x;

     //initialize private data members
     this->myZ = new int;
     *myZ = 0;
     this->myString = "";
}


myClass::myClass(const myClass& copyMe)
{
     myX = copyMe.myX;

     myZ = new int;
     myZ = copyMe.myZ;

     myString = copyMe.myString;
}

myClass& myClass::operator=(const myClass& rhs)
{
     if (this != &rhs){

          delete myX;
          delete myZ;

          myX = new int;
          myX = rhs.myX;

          myZ = new int;
          myZ = rhs.myZ;

          myString = rhs.myString;
     }
     return *this;
}

int main ()
{

    int n1 = 23;
    int n2 = 42;

    myClass A(&n1);
    myClass B(&n2);

    B = A;

    return 0;
}
0
 
AxterCommented:
I recommend against using pointers, unless you're really sure you need to.
Using pointers where there are not needed, only invites trouble.

For example, in the above code posted by jkr, a concrete type is being passed into the constructor.
int n2 = 42;
myClass B(&n2);

Now, when the assignment operator is called, that concrete type is being deleted.
 B = A;  //a delete will be performmed on address of n2


Looking at the posted code, there's no clear justification for using a pointer.

If you do need to use a pointer, then I recommend you use a smart pointer instead.

If you use smart pointers, then you don't need a copy constructor, or an assignment operator, because the default compiler generated version will work just fine.

As a general rule, I recommend designing your class so that you don't need a copy constructor or an assignment operators.
When your class needs to have a copy constructor and/or assignment operator, you increase the potential for adding bugs to your code when new data members are added to the class.
0
Free Tool: Site Down Detector

Helpful to verify reports of your own downtime, or to double check a downed website you are trying to access.

One of a set of tools we are providing to everyone as a way of saying thank you for being a part of the community.

 
rajeev_devinCommented:
One thing is missing. You should have a destructor if you are allocating memory dynamically.
myClass::~myClass()
{
   myX = NULL;
   delete myZ; // need to be deleted.
}
0
 
jkrCommented:
>>Now, when the assignment operator is called, that concrete type is being deleted.
>> B = A;  //a delete will be performmed on address of n2

Yes, I missed that. BTW, I'd recommend against pointers in that case also, but considered their usage as an assignment constraint.
0
 
waysideCommented:
Shouldn' t this be

myClass::myClass(const myClass& copyMe)
{
     myX = copyMe.myX;

     myZ = new int;
     *myZ = *copyMe.myZ;

     myString = copyMe.myString;
}

?

If you just do

     myZ = new int;
     myZ = copyMe.myZ;

you are copying the pointers, not the data pointed to by the pointers. And it creates a leak.
0
 
ThummalaRaghuveerCommented:
The copy assignment operator code is simply not good enough.

what happens if myX throws a bad allocation exception. You will be leaving the source object in a diastrous state.

>>         delete myX;
>>         delete myZ;

>>          myX = new int;
>>          myX = rhs.myX;


I would recommend using a temp object to copy rhs and then if everything is successful you can assign it to the object that needs to be changed. Thats the exception safe way of handling this issue. You dont even have to check for self assignment also. Take a look at this code.

myClass& myClass::operator=(const myClass& rhs)
{
    myClass temp(rhs);
    swap(temp);  // swap *this's data with the copy's
    return *this;
}

Hope this helps.

Raghuveer.

0
 
JohnSantaFeAuthor Commented:
Thanks everybody for the review.

I agree with the comments about not using pointers in this manner.  In my real code those are pointers to other classes and not integers.  I just wanted to keep the example simple.

Does anyone disagree with Wayside?  I think he's got a good point.  If I just copy the pointer it defeated the point of a copy constructor.

Amen to designing your code so you don't have to do this too much!



0
 
JohnSantaFeAuthor Commented:
DineshJolania

Thanks!

You made a change that I'd like to clarify.

The pointer int* x  gets passed into the constructor.  In my orginal examample, I did not new memory for it.  The pointer myX gets assigned the pointer x, and the myX variable lives on the stack.  Since I want a copied object to have the same myX pointer as the original (the reason for passing it in)[i.e. A->myX = B->myX]  I don't need to new memory and then dereference it in order to copy it. An important thing is I cannot delete the myX pointer in my destructor (but of course I wouldn't do that because I didn't new the memory for it)

Does this make any sense?  (Again, think of these as pointers to classes and not really integers)

Here's what I think the code should look like:


#include <string>
using namespace std;

class myClass
{
public:
     //constructor
     myClass(int* x);
     
     //copy constructor
     myClass(const myClass& copyMe);
     
     //assignment operator
     myClass& operator=(const myClass& rhs);
     
     ~myClass();

private:
     
     int* myX;
     int* myZ;
     string myString;
};


myClass::myClass(int* x)
{
     myX = x;  //No need to allocate memory for myX, we will not delete it
     
     //initialize private data members
     myZ = new int;
     *myZ = 0;
     myString = "";
}

myClass::~myClass()
{
     delete myZ;
     // do not delete myX
}

myClass::myClass(const myClass& copyMe)
{
     myX = copyMe.myX;  // For this variable, just copy the pointer, so we can use the object being pointed to
                                     //should not delete this pointer
     
     myZ = new int;
     *myZ = *(copyMe.myZ);  
     
     myString = copyMe.myString;
}
myClass& myClass::operator=(const myClass& rhs)
{
     if (this != &rhs){
         
          //do not delete myX
          delete myZ;
         
          myX = rhs.myX;  // Same here also
         
          myZ = new int;
          *myZ = *(rhs.myZ); // Same here also
         
          myString = rhs.myString;
     }
     return *this;
}

int main ()
{
     
    int n1 = 23;
    int n2 = 42;
     
    myClass A(&n1);
     
    myClass B(&n2);
     
    B = A;
     
    return 0;
}



0
 
DineshJolaniaCommented:
Yes I agree. This code looks fine to me.
As in original code posted here I could see "delete myX" in assignment operator function, i had suggested that change.
If you want that pointer variable just as placeholder and then you need not get into allocation/deallocation.

Thanks
DJ
0
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

All Courses

From novice to tech pro — start learning today.