Is returning references of member variables bad practice?

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

Solution 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.

Solution 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.

Solution 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.