1

I am trying to improve my understanding of the copy constructor and copy assign. operator Here is a simple class that I came up with

class Bar
{
    char* name;
    int zip;
    std::string address;

    public:
    Bar(const Bar& that)
    {
            //Copy constructor
            size_t len = strlen(that.name + 1);
            name = new char[len];
            strcpy(name, that.name);

            //Copy the zip
            this->zip = that.zip;

            //Copy the address
            this->address = that.address;
    }
    Bar& operator=(const Bar& that)
    {
        //Assignment operator
        if(this != &that)
        {
            //Copy the name
            size_t len = strlen(that.name + 1);
            name = new char[len];
            strcpy(name, that.name);

            //Copy the zip
            this->zip = that.zip;

            //Copy the address
            this->address = that.address;

        }
        return *this;
    }
};

My question is since the code in the copy constructor and copy assignment operator are the same does it make more sense to unify that into a deep copy method so that incase I add another member variable I dont have to add another line to the copy cnstr and copy assign. section ? Any suggestions ?

Rajeshwar
  • 11,179
  • 26
  • 86
  • 158

2 Answers2

2

The "normal" way of doing things where you manage your own resources is a little different:

char* cppstrdup(const char*s, int len=0);
class Bar
{
    char* name;
    int zip;
    std::string address;    
public:
    Bar(const Bar& that)
        :name(nullptr),
        zip(that->zip),
        address(that->address)
    {
        name = cppstrdup(that.name); //done here for exception safety reasons
    }
    Bar(Bar&& that) //if you have C++11 then you'll want this too
        :name(nullptr)
    {
        swap(*this,that);
    }
    ~Bar() //you forgot the destructor
    {
        delete [] name;
    }
    Bar& operator=(Bar that) //this is called copy and swap.
    { //"that" is a copy (notice, no & above), and we simply swap
        swap(*this,that);
        return *this;
    }
    friend void swap(Bar& left, Bar& right)
    {
        using std::swap;
        swap(left.name, right.name);
        swap(left.zip, right.zip);
        swap(left.address, right.address);
    }
};
//uses new instead of malloc
inline char* cppstrdup(const char* s, int len)
{
     if (s==0) return nullptr;
    if (len==0) len = strlen(s);
    char* r = new char[len+1];
    strncpy(r, len+1, s);
    r[len] = 0;
    return r;
}

The benefits of this pattern is that it is much easier to get exception safety, often with the strong exception guarantee.

Of course, even more normal is to not use char* name, and obey the "Rule of Zero" isntead. In which case, it becomes VERY different: 

class Bar
{
    std::string name;
    int zip;
    std::string address;

public:
    Bar() = default; //well, that's easy
};
Mooing Duck
  • 64,318
  • 19
  • 100
  • 158
  • If you are going to add C++11 to it could you also give out the working of move assignment operator – Rajeshwar Apr 18 '14 at 20:24
  • @Rajeshwar: Yes, I thought that move constructor was too simple. Good catch. `swap` as a friend keeps it out of the global namespace, which keeps error messages simpler, and also gives it access to the private members which it needs to work. – Mooing Duck Apr 18 '14 at 20:28
  • Thanks that makes sense. One last question if `Bar` was derived and had a base class say `BaseBar` How would we handle that in swap method ? – Rajeshwar Apr 18 '14 at 20:36
  • 1
    "The benefits of this pattern is that it is entirely exception safe" ...except for leaking `name` when the copy constructor of `address` throws an exception. – Casey Apr 18 '14 at 20:44
  • 1
    You've defined a non-member binary swap, but in the assignment operator you call a unary swap – Jonathan Wakely Apr 18 '14 at 20:45
  • @MooingDuck could you also answer this http://stackoverflow.com/questions/23162091/friend-function-inside-code – Rajeshwar Apr 18 '14 at 20:50
  • @Casey: Whooops. Fixed, at least for this case. – Mooing Duck Apr 18 '14 at 21:14
  • in case of an assignment operator you have to handle the deep copy of the base class yourself. So if Bar was derived and had a base class say BaseBar How would we handle that in swap method since the assignment operator calls swap ? – Rajeshwar Apr 18 '14 at 21:21
  • 1
    simply add the line `swap((basebar&)(*this), that);` inside of `swap`. (Since the left is a `basebar&` and the right is a `base`, C++ will select `swap(basebar&,basebar&)`, so we really only need to cast one side manually`). – Mooing Duck Apr 18 '14 at 21:25
  • `name` needs to be deallocated with `free` instead of `delete[]` - and then I promise to stop nitpicking. – Casey Apr 18 '14 at 21:50
  • @Casey: OP was using `new`, so my use of `strdup` was incorrect. Fixed. If anything else needs nitpick'd, let me know. Rather be bothered than have a bad answer. – Mooing Duck Apr 18 '14 at 21:56
0

Check COPY & SWAP idiom. In short - your logic goes into copy constructor and swap method and your assignment operator looks like:

Bar& operator=(const Bar& that)
{
    Bar temp(that);
    swap(*this, temp);
    return *this;
}
Tomek
  • 4,554
  • 1
  • 19
  • 19