Copy Constructors and calling functions

Question

Hello,

I'm trying to call an accessor function in a copy constructor but it's not working. Here's an example of my problem:

A.h

class A {

public:
    //Constructor
    A(int d);
    //Copy Constructor
    A(const A &rhs);

    //accessor for data
    int getData();

    //mutator for data
    void setData(int d);

private:
    int data;
};

A.cpp

#include "A.h"

//Constructor
A::A(int d) {
    this->setData(d);
}

//Copy Constructor
A::A(const A &rhs) {
    this->setData(rhs.getData()); 
}

//accessor for data
int A::getData() {
    return data;
}

//mutator for data
void A::setData(int d) {
    data = d;
}

When I try to compile this, I get the following error:

error: passing 'const A' as 'this' argument of 'int A::getData()' discards qualifiers

If I change rhs.getData() to rhs.data, then the constructor works fine. Am I not allowed to call functions in a copy constructor? Could somebody please tell me what I'm doing wrong?

Thanks,

helixed

Answer 1

The problem is rhs is declared as const, but getData() isn't, so it could be modifying rhs when you call it even though rhs is supposedly const. As getData() is an accessor, it should be const too:

//accessor for data
int getData() const;

Answer 2

Your "accessor" can only be called on non-const objects, because it isn't marked const. You should declare it:

int getData() const;

Then you're allowed to call it on rhs, which is a const reference.

Answer 3

If you insist on having an accessor function in the first place (a lousy idea in general), it should at least be const:

int A::getData() const { 
    return data;
}

This sort of thing is also a prime candidate for being inline, so you might as well define it in the class definition.

Edit: the reason accessors are a bad idea in general is that they're generally a pretty lousy abstraction. In fact, they're not usually an abstraction at all -- they're a dis traction. Consider the ones you posted: neither the accessor nor the mutator does anything useful at all. All your accessor and mutator are doing is producing public data with really ugly syntax. Worse they require that all client code know about the implementation detail that you've used functions to give access to this data.

Years ago I spent a couple of weeks sifting through bunches of code looking at this very situation. My conclusion was pretty simple: around 90-95% of accessors and mutators are just like the ones in the code you posted -- public data with ugly syntax.

That leaves some single-digit percentage that might have done something useful. Of those I sorted through to be certain exactly what they were really doing, most were doing nothing more than creating a variable with a limited range (e.g. an integer between 1 and 100). For those, you'd be better off using a small template for exactly that purpose -- and getting rid of the ugly syntax by using a "setter" named operator= and a getter named operator T for some appropriate base type T (usually int or unsigned int).

That leaves some minuscule percentage where the accessor/mutator were there to enforce something like a relationship between two variables within the class -- and in those cases, it was usually only an accessor, not a mutator (at least directly).

I reiterate, however: those constituted fewer than 1% of all the acessors and mutators I looked at. In fact, nearly the only instances (when you get down to it) were containers that made their current size visible via an accessor. For these, though the exact form is slightly different, I think the same basic idea as above should generally be used: a public object that overloads operator T to give read-only access to the data. The biggest difference is that this template takes an extra parameter for a type that it'll make its friend, so that type will read/write access instead of the read-only granted to the rest of the world. The general idea looks like this:

template <class T, class owner> 
class value { 
     T data;
     friend class owner;
     value &operator=(T const &t) { data = t; }
     value(T const &init) : data(init) {}
public:
     operator T() const { return data; }
};

In reality, the details can (and do) get a bit messy, mostly because the "owner" type usually isn't really a type at all, it's a template itself, so that has to be turned into a template-template parameter. The other detail that would change in a real implementation is that this often includes some range limits as well -- again, not a big deal, but varies with usage.