Want to protect your cyber security and still get fast solutions? Ask a secure question today.Go Premium

x
?
Solved

Copy constructor - assignment operator example

Posted on 2006-05-17
10
Medium Priority
?
393 Views
Last Modified: 2010-04-01
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;
}
0
Comment
Question by:JohnSantaFe
  • 2
  • 2
  • 2
  • +4
10 Comments
 
LVL 86

Assisted Solution

by:jkr
jkr earned 300 total points
ID: 16705374
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
 
LVL 30

Assisted Solution

by:Axter
Axter earned 300 total points
ID: 16705690
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
 
LVL 12

Assisted Solution

by:rajeev_devin
rajeev_devin earned 200 total points
ID: 16705699
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
Technology Partners: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
LVL 86

Expert Comment

by:jkr
ID: 16705717
>>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
 
LVL 14

Assisted Solution

by:wayside
wayside earned 400 total points
ID: 16710930
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
 
LVL 2

Assisted Solution

by:ThummalaRaghuveer
ThummalaRaghuveer earned 200 total points
ID: 16711160
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
 

Author Comment

by:JohnSantaFe
ID: 16713791
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
 
LVL 3

Accepted Solution

by:
DineshJolania earned 600 total points
ID: 16718648
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
 

Author Comment

by:JohnSantaFe
ID: 16720541
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
 
LVL 3

Expert Comment

by:DineshJolania
ID: 16721252
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

Featured Post

Free Tool: Subnet Calculator

The subnet calculator helps you design networks by taking an IP address and network mask and returning information such as network, broadcast address, and host range.

One of a set of tools we're offering as a way of saying thank you for being a part of the community.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

This article will show you some of the more useful Standard Template Library (STL) algorithms through the use of working examples.  You will learn about how these algorithms fit into the STL architecture, how they work with STL containers, and why t…
IntroductionThis article is the second in a three part article series on the Visual Studio 2008 Debugger.  It provides tips in setting and using breakpoints. If not familiar with this debugger, you can find a basic introduction in the EE article loc…
The viewer will learn how to user default arguments when defining functions. This method of defining functions will be contrasted with the non-default-argument of defining functions.
The viewer will be introduced to the technique of using vectors in C++. The video will cover how to define a vector, store values in the vector and retrieve data from the values stored in the vector.

580 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question