Is this a good design in C++?

Question

Is it a good idea to replace getters and setters with a single function returning a reference? For example, instead of this,

class Person {
  std::string name;
public:
  std::string GetName() { return this->name; }
  void SetName(std::string& toSet) { this->name = toSet; } 
};

Is this a good idea,

class Person {
  std::string name;
public:
  std::string& Name() { return this->name; }
};
...
...
Person p;
p.Name() = "Joe"; // Write. 
std::string local = p.Name(); // Read.

Does the second code have some problem?

But in C#, methods return object by reference. For example,

class Person
{
    string name;
    public Person(string _name) { this.name = _name;  }
    public string GetName()
    {
        return this.name;
    }
    public void SetName(string _name)
    {
        this.name = _name;
    }
};

class Company
{
    Person manager;
    public Company()
    {
        this.manager = new Person("Mr. K");
    }

    // The Person object is returned by reference.
    // This is equivalent to "Person& GetManager()" in C++.
    public Person GetManager() 
    {
        return this.manager;
    }
};
...
...
Company comp = new Company();

// Equivalent to,
// Person& p = comp.GetManager();
Person p = comp.GetManager(); // p is a reference to comp.manager
p.SetName("Mr. L");  // This statement changes comp.manager.name

Console.WriteLine(comp.GetManager().GetName()); // Prints "Mr. L"

In the above code, comp.manager.name wouldn't have changed if the method GetManager() returned Person by value. So, this code isn't dependent on return value optimization.
Isn't this code a violation of encapsulation?

Answer 1

The second code is conceptually the same as providing public access to the member variable.

It also has the exact same drawbacks: You can't change your mind on how a person's name will be stored and you can't control how the property will be used.
You can't change the implementation, because to return a reference, you need an actual std::string object to return that remains valid.

In other words, by returning a reference, you are breaking encapsulation.

---

If you are worried about the efficiency of returning by value, the large majority of modern compilers implemet RVO (return value optimisation) and will heavily optimize returning stuff by value to avoid making copies that will be destructed immediately thereafter.

If you are using a compiler that doesn't have RVO, you can return a const reference. That doesn't give the same drawback, because the user of your class can't use such a reference to change your internals.
Also, if you find that you need to change how stuff is stored internally in the class, you can change the return type to returning a value without (significantly) affecting the users of the class.

Answer 2

Rather than debating over the details of how to give the outside world access to the person's name, you should (IMO) be thinking about how to design the class so the outside world doesn't need access to something internal to the class.

If you want to support printing out the person's name, consider overloading operator<< for the class, and provide some manipulators to control what's printed out and in what format.

If you want to support something like sorting on name, then provide a comparison operator that handles that directly.

The better design isn't the one that gives access to the class' internals the most cleanly. It's the one that renders such access redundant.

Answer 3

Does the second code have some problem?

Yes, two.

1. You are making it into the equivalent of a public field and thereby fixing and revealing the implementation. It breaks encapsulation and locks you in.

2. You miss the opportunity to ensure that your object's state is always valid, especially on the setter.

In my view the only legitimate reason to use a setter is that it should be viewed as an instruction to the object: "Please change your name to be XXX". The object should actively participate in the change, by (for example) parsing the name, checking it for validity, checking that it is consistent with other internal state, and perhaps by raising an exception if it is invalid. In debug mode you can also run extended validation checks on both getter and setter. When you expose the object itself you lose all that.

But of course getters and setters are evil. Really you should change object state only by asking the object to carry out some function, to which the internal state is merely incidental.

---

In response to your amended question, yes your example is a breach of encapsulation. Any time you ask an object for access to some other object you assume knowledge and you violate tell-dont-ask. Whether that matters depends on the semantics and in your example, it matters.