C++: Copy contructor: Use Getters or access member vars directly?

Question

Have a simple container class:

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;
}

So, would you recommend this method or accessing the member variables directly?

• 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

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 2

If your getters are (inline and) not virtual, there's no pluses nor minuses in using them wrt direct member access -- it just looks goofy to me in terms of style, but, no big deal either way.

If your getters are virtual, then there is overhead... but nevertheless that's exactly when you DO want to call them, just in case they're overridden in a subclass!-)

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

Ask yourself what the costs and benefits are.

Cost: higher runtime overhead. Calling virtual functions in ctors is a bad idea, but setters and getters are unlikely to be virtual.

Benefits: if the setter/getter does something complicated, you're not repeating code; if it does something unintuitive, you're not forgetting to do that.

The cost/benefit ratio will differ for different classes. Once you're ascertained that ratio, use your judgment. For immutable classes, of course, you don't have setters, and you don't need getters (as const members and references can be public as no one can change/reseat them).

Answer 6

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 7

There is a simple test that works for many design questions, this one included: add side-effects and see what breaks.

Suppose setter not only assigns a value, but also writes audit record, logs a message or raises an event. Do you want this happen for every property when copying object? Probably not - so calling setters in constructor is logically wrong (even if setters are in fact just assignments).

Answer 8

Although I agree with other posters that there are many entry-level C++ "no-no's" in your sample, putting that to the side and answering your question directly:

In practice, I tend to make many but not all of my member fields* public to start with, and then move them to get/set when needed.

Now, I will be the first to say that this is not necessarily a recommended practice, and many practitioners will abhor this and say that every field should have setters/getters.

Maybe. But I find that in practice this isn't always necessary. Granted, it causes pain later when I change a field from public to a getter, and sometimes when I know what usage a class will have, I give it set/get and make the field protected or private from the start.

YMMV

RF

• you call fields "variables" - I encourage you to use that term only for local variables within a function/method

Answer 9

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.