Is using a reference as a return value considered as bad coding style?

Question

I got a question regarding to these two possibilities of setting a value:

Let's say I got a string of a class which I want to change. I am using this:

void setfunc(std::string& st) { this->str = st; }

And another function which is able to do the exact same, but with a string reference instead of a void for setting a value:

std::string& reffunc() { return this->str; }

now if I am going to set a value I can use:

std::string text("mytext");
setfunc(text);
//or
reffunc() = text;

Now my question is if it is considered bad at using the second form of setting the value. There is no performance difference.

Answer 1

The reason to have getters and setters in the first place is that the class can protect its invariants and is easier to modify.

If you have only setters and getters that return by value, your class has the following freedoms, without breaking API:

• Change the internal representation. Maybe the string is stored in a different format that is more appropriate for internal operations. Maybe it isn't stored in the class itself.
• Validate the incoming value. Does the string have a maximum or minimum length? A setter can enforce this.
• Preserve invariants. Is there a second member of the class that needs to change if the string changes? The setter can perform the change. Maybe the string is a URL and the class caches some kind of information about it. The setter can clear the cache.

If you change the getter to return a const reference, as is sometimes done to save a copy, you lose some freedom of representation. You now need an actual object of the return type that you can reference which lives long enough. You need to add lifetime guarantees to the return value, e.g. promising that the reference is not invalidated until a non-const member is used. You can still return a reference to an object that is not a direct member of the class, but maybe a member of a member (for example, returning a reference to the first name part of an internal name struct), or a dereferenced pointer.

But if you return by non-const reference, almost all bets are off. Since the client can change the value referenced, you can no longer rely on a setter being called and code controlled by the class being executed when the value changes. You cannot constrain the value, and you cannot preserve invariants. Returning by non-const reference makes the class little different from a simple struct with public members.

Which leads us to that last option, simply making the data member public. Compared to a getter returning a non-const reference, the only thing you lose is that the object returned can no longer be an indirect member; it has to be a direct, real member.

On the other side of that equation is performance and code overhead. Every getter and setter is additional code to write, with additional opportunities for errors (ever copy-pasted a getter/setter pair for one member to create the same for another and then forgot to change one of the variables in there?). The compiler will probably inline the accessors, but if it doesn't, there's call overhead. If you return something like a string by value, it has to be copied, which is expensive.

It's a trade-off, but most of the time, it's better to write the accessors because it gives you better encapsulation and flexibility.

Answer 2

We cannot see the definition of the member str. If it's private, your reffunc() exposes a reference to a private member; you're writing a function to violate your design, so you should reconsider what you're doing. Moreover, it's a reference, so you have to be sure that the object containing str still exists when you use that reference. Moreover, you are showing outside implementation details, that could change in the future, changing the interface itself (if str becomes something different, setfunc()'s implementation could be adapted, reffunc()'s signature has to change).

It's not wrong what you wrote, but it could be used in the wrong way. You're reducing the encapsulation. It's a design choice.

Answer 3

It's fine. However, you have to watch out for these pitfalls:

• the referenced object is modifiable. When you return a non-const reference, you expose data without protection against modifications. Obvious, but be aware of this anyway!

• referenced objects can go out of sope. If the referenced object's lifetime ends, accessing the reference will be undefined behavior. However, they can be used to extend the lifetime of temporaries.

Answer 4

The way you used the reffunc() function is considered bad coding. But (as mentioned in the comments), generally speaking, returning references is not bad coding.

Here's why reffunc() = text; is considered bad coding:

People usually do not expect function calls on the left hand of an assignment, but on the right side. The natural expectation when seeing a function call is that it computes and returns a value (or rvalue, which is expected to be on the right hand side of assignment) and not a reference (or lvalue, which is expected to be on the left hand side of assignment).

So by putting a function call on the left hand side of the assignment, you are making your code more complicated, and therefore, less readable. Keeping in mind that you do not have any other motivations for it (as you say, performance is the same, and it usually is in these situations), good coding recommends that you use a "set" function.

Read the great book "Clean Code" for more issues on clean coding.

As for returning references in functions, which is the title of your question, it is not always bad coding and is sometimes required for having cleaner and briefer code. Specifically many operator overloading features in c++ work properly if you return a reference (see operator[] in std::vector and the assignment operator which usually help the code become more readable and less complex. See the comments).