Is this good code? (copy ctor + operator=)

Question

For one reason or another, I'm forced to provide both a copy constructor and an operator= for my class. I thought I didn't need operator= if I defined a copy ctor, but QList wants one. Putting that aside, I hate code duplication, so is there anything wrong with doing it this way?

Fixture::Fixture(const Fixture& f) {
    *this = f;
}

Fixture& Fixture::operator=(const Fixture& f) {
    m_shape         = f.m_shape;
    m_friction      = f.m_friction;
    m_restitution   = f.m_restitution;
    m_density       = f.m_density;
    m_isSensor      = f.m_isSensor;
    return *this;
}

And just out of curiosity, there's no way to switch it so that the bulk of the code is in the copy ctor and operator= somehow utilizes it? I tried return Fixture(f); but it didn't like that.

---

It appears I need to make it more clear that the copy constructor and assignment operator have been implicitly disabled by the class I am inheriting from. Why? Because it's an abstract base class that shouldn't be instantiated on its own. This class, however, is meant to stand alone.

Answer 1

I think you should initialize your object member variables using initializer list. If your variables are of primitive-types, then it doesn't matter. Otherwise, assignment is different from initialization.

---

You could do it with a little trick by initializing pointers inside the copy constructor to 0, then you could call delete safely in the assignment operator:

Fixture::Fixture(const Fixture& f) : myptr(0) {
    *this = f;
}
Fixture& Fixture::operator=(const Fixture& f) {
    // if you have a dynamic array for example, delete other wise.
    delete[] myptr;
    myptr = new int[10];
    // initialize your array from the other object here.
    ......
    return *this;
}

Answer 2

Copy ctor and assignment are entirely distinct -- assignment typically needs to free resources in the object that it's replacing, copy ctor is working on a not-yet-initialized object. Since here you apparently have no special requirements (no "releasing" needed on assignment), your approach is fine. More in general, you might have a "free all resources the object is holding" auxiliary method (to be called in the dtor and at the start of assignment) as well as the "copy these other things into the object" part that's reasonably close to the work of a typical copy ctor (or mostly, anyway;-).

Answer 3

I think you run into issues if your operator= ever becomes virtual.

I would recommend writing a function (maybe static) that does the copy then have the copy-constructor and operator= call that function instead.

Answer 4

This is bad, because the operator= can't rely on a set-up object anymore. You should do it the other way around, and can use the copy-swap idiom.

In the case where you just have to copy over all elements, you can use the implicitly generated assignment operator.

In other cases, you will have to do something in addition, mostly freeing and copying memory. This is where the copy-swap idiom is good for. Not only is it elegant, but it also provide so an assignment doesn't throw exceptions if it only swaps primitives. Let's a class pointing to a buffer that you need to copy:

Fixture::Fixture():m_data(), m_size() { }

Fixture::Fixture(const Fixture& f) {
    m_data = new item[f.size()];
    m_size = f.size();
    std::copy(f.data(), f.data() + f.size(), m_data);
}

Fixture::~Fixture() { delete[] m_data; }

// note: the parameter is already the copy we would
// need to create anyway. 
Fixture& Fixture::operator=(Fixture f) {
    this->swap(f);
    return *this;
}

// efficient swap - exchanging pointers. 
void Fixture::swap(Fixture &f) {
    using std::swap;
    swap(m_data, f.m_data);
    swap(m_size, f.m_size);
}

// keep this in Fixture's namespace. Code doing swap(a, b)
// on two Fixtures will end up calling it. 
void swap(Fixture &a, Fixture &b) {
  a.swap(b);
}

That's how i write the assignment operator usually. Read Want speed? Pass by value about the unusual assignment operator signature (pass by value).

Answer 5

Yes, this is good practice and should (almost) always be done. In addition toss in a destructor and default constructor (even if you make it private).

In James Coplien's 1991 book Advanced C++, this is described as part of "Orthodox Canonical Form". In it, he advocates for a default constructor, a copy constructor, the assignment operator and a destructor.

In general, you must use the orthodox canonical form if:

• You want to support assignment of object of the class, or want to pass those objects as call-by-value parameters to a function, and
• The object contains pointers to objects that are reference-counted, or the class destructor performs a delete on a data member of the object.

You should use the orthodox canonical form for any nontrivial class in a program, for the sake of uniformity across classes and to manage the increasing complexity of each class over the course of program evolution.

Coplien offers pages of reasons for this pattern and I couldn't do them justice here. However, a key item that has already been touched on is the ability to clean up the object that is being overwritten.

Answer 6

If you re-write your

Fixture& Fixture::operator=(const Fixture &f) {
    *this = Fixture(f);
    return *this;
}

as

Fixture& Fixture::operator=(const Fixture &f) {
    Fixture* p = new( this ) Fixture( f );
    return *p;
}

This then uses "placement new" to construct the this pointer.

Answer 7

You're simply doing a member-wise copy and assignment in your examples. This is not something you need to write yourself. The compiler can generate implicit copy and assignment operations that do exactly that. You only need to write your own copy constructor, assignment and/or destructor if the compiler-generated ones are not appropriate (i.e. in case you manage some resource through a pointer or something like that)