Is returning references of member variables bad practice?

Question

The following is said to be better than having First() and Second() as public members. I believe this is nearly as bad.

// Example 17-3(b): Proper encapsulation, initially with inline accessors. Later
// in life, these might grow into nontrivial functions if needed; if not, then not.

template<class T, class U>
class Couple {
public:
  Couple()           : deleted_(false) { }
  void MarkDeleted() { deleted_ = true; }
  bool IsDeleted()   { return deleted_; }

private:
 T first_;
 U second_;
 bool deleted_;
 T& First()         { return first_; }
 U& Second()        { return second_; }
};

If you're giving a way to access a private variable outside of the class then what's the point? Shouldn't the functions be

T First(); void(or T) First(const T&)

Quad

Answer 1

There are several reasons why returning references (or pointers) to the internals of a class are bad. Starting with (what I consider to be) the most important:

1. Encapsulation is breached: you leak an implementation detail, which means that you can no longer alter your class internals as you wish. If you decided not to store first_ for example, but to compute it on the fly, how would you return a reference to it ? You cannot, thus you're stuck.

2. Invariant are no longer sustainable (in case of non-const reference): anybody may access and modify the attribute referred to at will, thus you cannot "monitor" its changes. It means that you cannot maintain an invariant of which this attribute is part. Essentially, your class is turning into a blob.

3. Lifetime issues spring up: it's easy to keep a reference or pointer to the attribute after the original object they belong to ceased to exist. This is of course undefined behavior. Most compilers will attempt to warn about keeping references to objects on the stack, for example, but I know of no compiler that managed to produce such warnings for references returned by functions or methods: you're on your own.

As such, it is usually better not to give away references or pointers to attributes. Not even const ones!

For small values, it is generally sufficient to pass them by copy (both in and out), especially now with move semantics (on the way in).

For larger values, it really depends on the situation, sometimes a Proxy might alleviate your troubles.

Finally, note that for some classes, having public members is not so bad. What would be the point of encapsulating the members of a pair ? When you find yourself writing a class that is no more than a collection of attributes (no invariant whatsoever), then instead of getting all OO on us and writing a getter/setter pair for each of them, consider making them public instead.

Undamped

+1, but there is a counter rationale for providing accessors: you can add instrumentation to the call and that makes it easier to find places where the object is being updated in the code, or attach a debugger to detect issues. – Fail

For you information the example was talking about classes which has private members hence the bool deleted_;. I like POD – Quad

@David: I am not against accessors as in T get()/void set(T), in which you can effectively track the changes of your values, provide lazy computation and plenty of other things, but the only interesting tidbit in a T& access() is to "trace" that an access was made, which does not bring much to the table. – Undamped

@acidzombie24: there is nothing preventing you to have some members publics and other privates. – Undamped

Why "it is usually better not to give away references or pointers to attributes. Not even const ones"? – Endocardial

@Pierre: As per the arguments above this sentence, specifically (1) Encapsulation and (3) Lifetime. – Undamped

This ignores what I believe to be good usage, in that it makes it clearer who is supposed to manage the underlying structure. For instance, let's say you have a parent class that has a vector of objects. If the member is protected, it's easy for derived classes to not understand that they do not manage the objects. By making the member private, but adding a protected method that returns a const reference, it's clear what class manages the list, and you can define an interface to update the collection. – Biisk

@bpeikes: Indeed, this answer ignores inheritance, and quite intentionally so. First because the OP doesn't mention it, so there seems no point in adding it in, and second because any advice that applies to the public/private boundary applies equally well to the protected/private boundary anyway. Your very example can be reworded without inheritance, talking about public vector vs private vector with const getter. And the same 3 issues I brought up apply then, starting with encapsulation preventing the class to switch to deque if it would be better, now that all users expect a vector. – Undamped

Answer 2

If template types T and U are big structures then return by value is costly. However you are correct that returning by reference is equivalent to giving access to a private variable. To solve both issues, make them const references:

const T& First()  const  { return first_; }
const U& Second() const  { return second_; }

P.S. Also, it's a bad practice to keep variables uninitialized inside constructor, when there is no setter method. It seems that in the original code, First() and Second() are wrappers over first_ and second_ which were meant for read/write both.

Genealogy

ah ha, good solution. As long as reads (First()) has so side effects this would be completely fine (not an unreasonable restriction but still a restriction :)) – Quad

This link has some interesting insights into pass by value and R-values. – Urticaceous

Answer 3

The answer depends on what one is trying to do. Returning references are a convenient way to facilitate mutation of data structures. A good example is the stl map. It returns reference to the element i.e.

std::map<int,std::string> a;
a[1] = 1;

nothing to stop you from doing

auto & aref = a[1];

Is it necessarily a bad practice? I would not think so. I would say, if you can do without it do so. If it makes life more convenient and efficient use it and be aware of what you are doing.

Pede