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&)

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.