Is this good code? (copy constructor and assignment 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

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 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;-).