Is Returning References of Member Variables Bad Practice

Is returning references of member variables bad practice?

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:

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

Is the practice of returning a C++ reference variable evil?

In general, returning a reference is perfectly normal and happens all the time.

If you mean:

int& getInt() {
    int i;
    return i;  // DON'T DO THIS.
}

That is all sorts of evil. The stack-allocated i will go away and you are referring to nothing. This is also evil:

int& getInt() {
    int* i = new int;
    return *i;  // DON'T DO THIS.
}

Because now the client has to eventually do the strange:

int& myInt = getInt(); // note the &, we cannot lose this reference!
delete &myInt;         // must delete...totally weird and  evil

int oops = getInt(); 
delete &oops; // undefined behavior, we're wrongly deleting a copy, not the original

Note that rvalue references are still just references, so all the evil applications remain the same.

If you want to allocate something that lives beyond the scope of the function, use a smart pointer (or in general, a container):

std::unique_ptr<int> getInt() {
    return std::make_unique<int>(0);
}

And now the client stores a smart pointer:

std::unique_ptr<int> x = getInt();

References are also okay for accessing things where you know the lifetime is being kept open on a higher-level, e.g.:

struct immutableint {
    immutableint(int i) : i_(i) {}

    const int& get() const { return i_; }
private:
    int i_;
};

Here we know it's okay to return a reference to i_ because whatever is calling us manages the lifetime of the class instance, so i_ will live at least that long.

And of course, there's nothing wrong with just:

int getInt() {
   return 0;
}

If the lifetime should be left up to the caller, and you're just computing the value.

Summary: it's okay to return a reference if the lifetime of the object won't end after the call.

How come we can return references to member variables of a class

You are confusing scope and lifetime. Those are two related, but ultimately different concepts.

The scope of the name x is the class scope. You can only use the unqualified name x for the member inside members functions (and some other minutia, irrelevant here) of the class.

The lifetime of the member object named x is the same as the enclosing class object. In this case, the lifetime of obj.x is the same as obj. Since you return a reference to an object within its lifetime, everything checks out.

The reason for your confusion may stem from learning that objects with automatic storage duration, like the following:

{
    int x;
}

Have their lifetime bound to their lexical scope (they can only be named inside those curly braces, their scope). But while it's true for those objects, it is not something that holds in general. Class objects can live independently of the scope of their name (as you saw). And some objects can have a lifetime but no scope and name. Consider:

auto* p = new Foo();

The new expression creates an object, but it has no name! So there is no scope to even speak of. Here p is the name of a pointer, not of the newly created object.

Is it a good idea to always return references for member variable getters?

There is no reason to return primitive types such as int and float by reference, unless you want to allow them to be changed. Returning them by reference is actually less efficient because it saves nothing (ints and pointers are usually the same size) while the dereferencing actually adds overhead.

Is it bad practice to reference class member object from within another member of the same class?

There are some guidelines, but they are generic, like use unique_ptr for owning pointers.

For the rest the guideline is: keep it simple. How did it come to this hierarchy with such an inter-dependency? Could there be a better way to structure your application?

Bidirectional references are possible but you need need to maintain them yourself, or make Outer non-copyable and non-movable:

class Outer {
    Some_type member;
    std::unique_ptr<Inner> inner;
public:
    Outer() {}

    Outer(Outer const& that) = delete; // makes Outer not copyable/movable
    Outer& operator=(Outer const& that) = delete;

    /* The only way to update inner, also sets the back-reference */
    void setInner(std::unique_ptr<Inner> ptr) noexcept {
        inner = std::move(ptr);
        if (inner) {
            inner->ptr_to_outer_member = &member;
        }
    }
};

If Outer is movable, it should actively maintain all back-references to itself to keep them in-sync.

For example like this:

class Some_type {
};
class Inner {
    Some_type* ptr_to_outer_member;
    friend class Outer; // so that Outer may reach the private ptr_to_outer_member
};

class Outer {
    Some_type member;
    std::unique_ptr<Inner> inner; // owning pointer to Inner
public:
    Outer() noexcept {
    }
    Outer(Outer&& that) noexcept {
        *this = std::move(that);
    }
    Outer& operator=(Outer&& that) noexcept {
        member = std::move(that.member);
        setInner(std::move(that.inner));
        return *this;
    }
    /* The only way to update inner, also sets the back-reference */
    void setInner(std::unique_ptr<Inner> ptr) noexcept {
        inner = std::move(ptr);
        if (inner) {
            inner->ptr_to_outer_member = &member;
        }
    }
};

int main() {
    Outer o;
    o.setInner(std::make_unique<Inner>());
    Outer o2(std::move(o)); // OK, move-construction
    Outer o3;
    o3 = std::move(o2); // OK, move-assignment
    Outer o4(o3); // error: Outer is not copyable
}

Having back-references in a movable type almost always requires a custom copy/move-constructor. Which means we have to also keep the rule of 3/5/0 in mind. In this example unique_ptr saves us from having a custom destructor. That may not always be the case.

Is returning by reference of a member variable EVER acceptable?

It's perfectly fine to return a reference to something that will outlive the function call. When you return an lvalue reference to a class data member, just make sure the class instance is itself an lvalue:

struct Foo
{
    X data;

    X & the_data() &   { return data; }
//                ^^^

While you're at it, you can also return the data as an rvalue if your instance is an rvalue:

    X && the_data() && { return std::move(data); }
};

Is it bad practice to return a non const reference to an element of a private vector member?

Is it bad practice to return a vector element by reference?

Yes, in general? In most cases, I would think if you took the time to encapsulate a member variable of a class as private in order to control the access and manipulation of that variable, designing a member function that will easily break that control renders the first step moot. This may not always be true depending on the use case, but here you pose the problem so abstractly that it is hard to give a specific answer. The only real problem I can identify in your post is this:

Returning an element by value is less efficient and since it is a copy, it is less efficient.

I guess the real question to ask here is there a meaningful, measurable performance difference between maintaining greater access control to the member variable versus having more direct access to the underlying memory so you can manipulate it faster? You are right that the return by reference is more efficient in some ways, but does that actually make a practical difference in your particular code?

Additionally, it also matters what level of data integrity you need to maintain for the private member variables you are exposing. einpoklum makes a great point that many standard containers follow this paradigm. They have no expectations on the values that are stored in the container, only that they maintain control over allocation/deletion of the memory held by them. Your class may have a stronger control requirements about what values the member values take. For example, if all the data elements in that vector needed to be non-negative, then by exposing a reference to that memory you lose the ability to have the class make those kind of data integrity guarantees. It really just depends on the requirements, although I prefer the paradigm of selectively releasing control over a member variable as needed rather than giving full access and slowly taking it away when you want to add additional guarantees.

Is returning a class member a bad practice?

Reterning a class method is perfectly fine if you want that class member to be readable outside the class, which is completely dependent upon what you're trying to accomplish. As for using a getter method, you already are; it is "good practice" to use such methods to create a more uniform, encapsulated setup for your class and therefore make it easier to edit the class later. Even if your function does other things (in this case, computes something in two dimensions), it's still a getter method.

In this particular case, however, you're not returning a value; you're returning a double pointer to a value. This means that the user will be able to edit the data (which I'm guessing is an array) ouside the class, which is NOT a good thing. You should perform a deep copy of the data before returning it.

As for using the garbage collector correctly: yes, you are. Although it's not called a garbage collector in C++, it's simply called freeing memory; a garbage collector is the name for a system that frees memory automatically.

Why can I expose private members when I return a reference from a public member function?

private does not mean "this memory may only be modified by member functions" -- it means "direct attempts to access this variable will result in a compile error". When you expose a reference to the object, you have effectively exposed the object.

Is it a bad practice to receive a returned private variable by reference ?

No, it depends on what you want. Things like std::vector<t>::operator[] would be quite difficult to implement if they couldn't return a non-const reference :) If you want to return a reference and don't want clients to be able to modify it, simply make it a const reference.

Does holding a C++ reference prevents variable of being destroyed?

Given that

MyShape::color is declared as a Color&

... the code is a bit problematic.

The thing to know about C++ references is that you can't re-seat them -- that is, unlike a C++ pointer, once a C++ reference is declared, it will always reference the object it was declared to reference, and cannot be re-directed to reference some other object instead. (Btw this is also the reason why you're not allowed to declare an unbound reference, e.g. MyColor & c; won't compile because it would be useless)

From that, it follows that what this code actually does:

void MyShape::setColor(const Color& newColor)
{
   this->color= newColor;
}

is overwrite the contents of (whatever Color object this->color is referencing) with the contents of newColor). It doesn't change the reference itself at all; rather it writes through the reference. (Remember that references are defined as aliases-to-something, so wherever you specify the reference, the program will behave as if you had specified the object-the-reference-is-referencing there instead)

To answer the titular question: holding a C++ reference does not guarantee that the object the reference is referencing will not be destroyed. It's quite possible to delete (or otherwise destroy) the referenced item, and end up with a "dangling reference" that will invoke Undefined Behavior if you try to use it.

As general advice, if you want to have a guard-value to represent "no-color", the best way to do it is to represent the guard-value as an actual color object (e.g. Color(0,0,0,0) could work, if your Color class has an alpha-channel, since any color with alpha=0 is transparent, hence no-color). That way there is no need to play games with pointers or references to try to represent that information. If that's not doable for some reason, have a look at std::optional as a way to represent "either-a-value-or-nothing".

As a final note, using pointers does not mean you have to use new and delete -- pointers work equally well (and you can shoot yourself in the foot with them equally badly) when they are pointing to stack-objects or member-objects. Which is to say, pointers are very powerful, and also very dangerous, and I wouldn't recommend using them for this purpose.

Is Returning References of Member Variables Bad Practice