Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
    
shouldn't this be at code review? –  Claudiu Aug 31 at 14:25
2  
@Claudiu no, most certainly not. Code Review is suitable when you've written some wonderful real-world code and want some feedback on how to improve it even further. CR is not for conceptual questions such as “Is design A or B better for use case C?” – which are on topic here. See CR's help center for more details. –  amon Aug 31 at 14:37
2  

3 Answers 3

up vote 11 down vote accepted

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.

share|improve this answer
    
It can be useful and efficient to return a const reference. If you do this, you should document how long the returned reference should be considered valid. –  aecolley Aug 31 at 14:10
    
The only thing that I find problematic in the first code is that the GetName() creates a copy of name. A std::string is of size 24 bytes in a 32-bit machine. But in C# and Java, in such cases, a reference is returned by default. –  Fish Aug 31 at 15:51
3  
If you are returning a reference, you might as well just make the thing public and be done with it. You are just making people do p.Name() = foo; rather than p.name = foo;. –  Steven Burnap Aug 31 at 16:46
1  
@Fish: See my edit –  Bart van Ingen Schenau Aug 31 at 17:24
1  
@Fish in Java, strings are immutable. This would be comparable to using a StringBuilder to model the state and returning a reference to that in the getter. –  Snowman Aug 31 at 22:45

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.

share|improve this answer
    
Thank you for your answer. This is not the code I am working on, this is just a contrived example. See my edit to the question. –  Fish Sep 1 at 15:41

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.

share|improve this answer
    
But in C#, every method returns by reference. Doesn't that violate these rules? –  Fish Sep 1 at 12:09
    
Sorry, I have no idea what that means. Can you please explain? –  david.pfx Sep 1 at 14:34
    
Thank you for your answer, see my edit to the question. –  Fish Sep 1 at 15:39

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.