C++: Copy constructor: Use getters or access membe

Question

I have a simple container class with a copy constructor.

Do you recommend using getters and setters, or accessing the member variables directly?

public Container 
{
   public:
   Container() {}

   Container(const Container& cont)          //option 1
   { 
       SetMyString(cont.GetMyString());
   }

   //OR

   Container(const Container& cont)          //option 2
   {
      m_str1 = cont.m_str1;
   }

   public string GetMyString() { return m_str1;}       

   public void SetMyString(string str) { m_str1 = str;}

   private:

   string m_str1;
}

• In the example, all code is inline, but in our real code there is no inline code.

Update (29 Sept 09):

Some of these answers are well written however they seem to get missing the point of this question:

• this is simple contrived example to discuss using getters/setters vs variables

• initializer lists or private validator functions are not really part of this question. I'm wondering if either design will make the code easier to maintain and expand.

• Some ppl are focusing on the string in this example however it is just an example, imagine it is a different object instead.

• I'm not concerned about performance. we're not programming on the PDP-11

Answer 1

Do you anticipate how the string is returned, eg. white space trimmed, null checked, etc.? Same with SetMyString(), if the answer is yes, you are better off with access methods since you don't have to change your code in zillion places but just modify those getter and setter methods.

Answer 2

I prefer using an interface for outer classes to access the data, in case you want to change the way it's retrieved. However, when you're within the scope of the class and want to replicate the internal state of the copied value, I'd go with data members directly.

Not to mention that you'll probably save a few function calls if the getter are not inlined.

Answer 3

EDIT: Answering the edited question :)

this is simple contrived example to discuss using getters/setters vs variables

If you have a simple collection of variables, that don't need any kind of validation, nor additional processing then you might consider using a POD instead. From Stroustrup's FAQ:

A well-designed class presents a clean and simple interface to its users, hiding its representation and saving its users from having to know about that representation. If the representation shouldn't be hidden - say, because users should be able to change any data member any way they like - you can think of that class as "just a plain old data structure"

In short, this is not JAVA. you shouldn't write plain getters/setters because they are as bad as exposing the variables them selves.

initializer lists or private validator functions are not really part of this question. I'm wondering if either design will make the code easier to maintain and expand.

If you are copying another object's variables, then the source object should be in a valid state. How did the ill formed source object got constructed in the first place?! Shouldn't constructors do the job of validation? aren't the modifying member functions responsible of maintaining the class invariant by validating input? Why would you validate a "valid" object in a copy constructor?

I'm not concerned about performance. we're not programming on the PDP-11

This is about the most elegant style, though in C++ the most elegant code has the best performance characteristics usually.

---

You should use an initializer list. In your code, m_str1 is default constructed then assigned a new value. Your code could be something like this:

class Container 
{
public:
   Container() {}

   Container(const Container& cont) : m_str1(cont.m_str1)
   { }

   string GetMyString() { return m_str1;}       
   void SetMyString(string str) { m_str1 = str;}
private:
   string m_str1;
};

---

@cbrulak You shouldn't IMO validate cont.m_str1 in the copy constructor. What I do, is to validate things in constructors. Validation in copy constructor means you you are copying an ill formed object in the first place, for example:

Container(const string& str) : m_str1(str)
{
    if(!valid(m_str1)) // valid() is a function to check your input
    {
        // throw an exception!
    }
}

Answer 4

You should use an initializer list, and then the question becomes meaningless, as in:

Container(const Container& rhs)
  : m_str1(rhs.m_str1)
{}

There's a great section in Matthew Wilson's Imperfect C++ that explains all about Member Initializer Lists, and about how you can use them in combination with const and/or references to make your code safer.

Edit: an example showing validation and const:

class Container
{
public:
  Container(const string& str)
    : m_str1(validate_string(str))
  {}
private:
  static const string& validate_string(const string& str)
  {
    if(str.empty())
    {
      throw runtime_error("invalid argument");
    }
    return str;
  }
private:
  const string m_str1;
};

Answer 5

As it's written right now (with no qualification of the input or output) your getter and setter (accessor and mutator, if you prefer) are accomplishing absolutely nothing, so you might as well just make the string public and be done with it.

If the real code really does qualify the string, then chances are pretty good that what you're dealing with isn't properly a string at all -- instead, it's just something that looks a lot like a string. What you're really doing in this case is abusing the type system, sort of exposing a string, when the real type is only something a bit like a string. You're then providing the setter to try to enforce whatever restrictions the real type has compared to a real string.

When you look at it from that direction, the answer becomes fairly obvious: rather than a string, with a setter to make the string act like some other (more restricted) type, what you should be doing instead is defining an actual class for the type you really want. Having defined that class correctly, you make an instance of it public. If (as seems to be the case here) it's reasonable to assign it a value that starts out as a string, then that class should contain an assignment operator that takes a string as an argument. If (as also seems to be the case here) it's reasonable to convert that type to a string under some circumstances, it can also include cast operator that produces a string as the result.

This gives a real improvement over using a setter and getter in a surrounding class. First and foremost, when you put those in a surrounding class, it's easy for code inside that class to bypass the getter/setter, losing enforcement of whatever the setter was supposed to enforce. Second, it maintains a normal-looking notation. Using a getter and a setter forces you to write code that's just plain ugly and hard to read.

One of the major strengths of a string class in C++ is using operator overloading so you can replace something like:

strcpy(strcat(filename, ".ext"));

with:

filename += ".ext";

to improve readability. But look what happens if that string is part of a class that forces us to go through a getter and setter:

some_object.setfilename(some_object.getfilename()+".ext");

If anything, the C code is actually more readable than this mess. On the other hand, consider what happens if we do the job right, with a public object of a class that defines an operator string and operator=:

some_object.filename += ".ext";

Nice, simple and readable, just like it should be. Better still, if we need to enforce something about the string, we can inspect only that small class, we really only have to look one or two specific, well-known places (operator=, possibly a ctor or two for that class) to know that it's always enforced -- a totally different story from when we're using a setter to try to do the job.

Answer 6

Why do you need getters and setters at all?

Simple :) - They preserve invariants - i.e. guarantees your class makes, such as "MyString always has an even number of characters".

If implemented as intended, your object is always in a valid state - so a memberwise copy can very well copy the members directly without fear of breaking any guarantee. There is no advantage of passing already validated state through another round of state validation.

As AraK said, the best would be using an initializer list.

---

Not so simple (1):
Another reason to use getters/setters is not relying on implementation details. That's a strange idea for a copy CTor, when changing such implementation details you almost always need to adjust CDA anyway.

---

Not so simple (2):
To prove me wrong, you can construct invariants that are dependent on the instance itself, or another external factor. One (very contrieved) example: "if the number of instances is even, the string length is even, otherwise it's odd." In that case, the copy CTor would have to throw, or adjust the string. In such a case it might help to use setters/getters - but that's not the general cas. You shouldn't derive general rules from oddities.