15

We can all agree on public variables being bad for encapsulation and all that. However, I noticed a lot of code that does this type of stuff:

class foo {
private:
    int integer_;
    string someString_;
    // other variables
public:
    int& integer() { return integer_; }
    string& someString() { return someString_; }
    // other "functions"
}

int main() {
    foo f;
    f.integer() = 10;
    f.someString() = "something";
    return 0;
}

I have seen this being used in many places and I don't get why. Basically it returns a reference to the data and thus exposes it directly to the outside. So encapsulation is not really achieved, not from any perspective.

Why is this commonly used?

Boann
  • 48,794
  • 16
  • 117
  • 146
Everyone
  • 1,751
  • 13
  • 36
  • 17
    _"Why is this commonly used?"_ Because of _common_ stupidity. – πάντα ῥεῖ Mar 03 '17 at 19:39
  • 12
    It's commonly used because many programmers don't really know what they are doing. –  Mar 03 '17 at 19:39
  • So they basically think they are solving a problem while all they are doing is just adding verbosity into their code? – Everyone Mar 03 '17 at 19:40
  • It's bad as other commenters already said. [Look here](http://stackoverflow.com/questions/2977007/public-data-members-vs-getters-setters) – mpiatek Mar 03 '17 at 19:40
  • 1
    Unfortunately common practice and best practice are not always the same. Btw I believe this is not common... – vadim Mar 03 '17 at 19:43
  • could you give some reference? I don't recall ever seeing such a code :) – luantkow Mar 03 '17 at 19:49
  • @Everyone The average programmer drinks too much coffee ;-) – πάντα ῥεῖ Mar 03 '17 at 19:53
  • "So they basically think they are solving a problem while all they are doing is just adding verbosity into their code?" - perfect description of getters and setters. – M.M Mar 04 '17 at 04:45
  • Related: [Does it make sense to provide non-const reference getter](https://stackoverflow.com/questions/4113845/does-it-make-sense-to-provide-non-const-reference-getter/4113864) – sergiol Mar 21 '18 at 18:37
  • Related: [Breaking encapsulation by returning non-const references to members](https://stackoverflow.com/questions/36855426/breaking-encapsulation-by-returning-non-const-references-to-members#comment86816406_36855426) – sergiol Apr 20 '18 at 11:34
  • Here is a Regex for Visual Studio which may help find cases of methods returning non-const references: `(?<!const\s+(?:\w+\s+)?|::)\b\w+(?:::\w+)*\s*(?:<[^<>]*>\s*)*&\s+\w+(?:::\w+)*\s*\(` — May be not perfect, someone can suggest improvements! – sergiol Apr 20 '18 at 11:34

5 Answers5

17

There's a recurring mantra, that getter/setter functions should be used to encapsulate your data. Hence many (unexperienced or coffee-overloaded) programmers get the idea they should use something like:

int& integer() { return integer_; }

but that isn't much different from simply writing:

class foo {
public: // <<<
    int integer_;
    string someString_;
    // ...
};

Well, it adds a function call, but you cannot control what the client does with the reference.


If you really want to provide a getter function write:

const int& integer() const { return integer_; }

A corresponding setter function looks like:

void integer(const int& value) {
    integer_ = value;
}
Boann
  • 48,794
  • 16
  • 117
  • 146
πάντα ῥεῖ
  • 1
  • 13
  • 116
  • 190
  • 1
    Yeah it just adds verbosity to the code that is unnecessary. Yet many use it. It just blows my mind :| – Everyone Mar 03 '17 at 19:45
  • Yeah, this is just like `size()` from `std::vector` – Everyone Mar 03 '17 at 19:48
  • 3
    Why would you not write: `int integer() const { return integer_; }`? –  Mar 03 '17 at 19:49
  • 2
    @NeilButterworth The difference is minor and my example extrapolates well for other types than `int` (besides keeping tight to OP's example). – πάντα ῥεῖ Mar 03 '17 at 19:50
  • @NeilButterworth Why not? you are not endangering the private variable for any changes. You are passing it as `const` – Everyone Mar 03 '17 at 19:55
  • 1
    @Everyone It's more typing, more code to read, and it is probably less efficient. If you look at similar functions in the Standard Library, they return integer values. –  Mar 03 '17 at 19:56
  • Just wanted to draw your attention to my own answer below, since I'm arguing with yours somewhat. – einpoklum Mar 03 '17 at 20:02
  • @πάνταῥεῖ So the code you just provided fairly looks like an explicit version of C#'s properties. – Everyone Mar 03 '17 at 20:03
  • @ein We're always _arguing_ somehow. I don't know what your point actually is, that it would conflict with my answer. – πάντα ῥεῖ Mar 03 '17 at 20:04
  • @Everyone Yes, that's the idiomatic way doing _properties_ in c++. – πάντα ῥεῖ Mar 03 '17 at 20:05
  • @πάνταῥεῖ: It conflicts with the sentence "that isn't much different than writing etc. etc." - my answer claims that it is different. – einpoklum Mar 03 '17 at 21:54
  • Having a public getter and setter is no different to having a public member, other than increased verbosity – M.M Mar 04 '17 at 04:46
10

I have to partially disagree both with @πάνταῥεῖ and @Rakete1111 's answers, considering how a class's definition is something that may evolve over time.

While it's true that, often, these getter methods are written by someone who's just heard the "no exposing members" mantra, they can also have legitimate uses:

  1. The getter method may later be modified to include some kind of validity check or resource allocation code before returning the reference - which direct access to the data member does not allow. While this means changing the class's code, it does not require changing the class user code. Also, if the implementation of the getter is not exposed in the class header, it might not even require recompiling the class user code. Note: Doing so is probably a sign of some other poor design choice.
  2. The getter method may be overridden by a subclass (in which case it is often made a virtual method) in a similar way to the above.
  3. The getter method may later replace its return type with a proxy for the original data member type, rather than a reference to an actual member - which may no longer even exist. Think of how vector<bool> works; when you call its operator[] you don't get a boolean&, you get some kind of proxy which, when assigned or assigned-to, does the appropriate bit extraction or setting.
  4. A non-const getter is not usable for non-const instances. So actually it does limit access relative to exposing the member outright. Whether the author of OP's example class actually intended this is a different question...

To sum up: The "dummy" non-const-reference getter can be a stub for other, meaningful, code.

That being said, it is often a good idea to just make the getter return a const reference or a value. Or just exposing the field in those cases where it's appropriate (and there are some of those too).

einpoklum
  • 118,144
  • 57
  • 340
  • 684
  • It really makes some sense what you are talking. I do realize C++ does not have C#'s properties which actually help controlling all these stuff easily and as you've said without the need to change user code. But again, comparing two different languages is not really valid – Everyone Mar 03 '17 at 20:01
  • Ever heard of YAGNI? –  Mar 03 '17 at 20:02
  • 2
    @NeilButterworth: (1) Yes, but - obligatory [link](https://en.wikipedia.org/wiki/You_aren't_gonna_need_it) for those who havent. (2) _I_ may not going to need it, but who knows what the author of that code is going to need? – einpoklum Mar 03 '17 at 20:04
  • (3) If you're designing an inheritance hierarchy, you may already need it (or at least - have use for it). (4) I've never actually written such getters myself :-) – einpoklum Mar 03 '17 at 20:05
  • @ein _"The getter method may later be modified to include some kind of validity check"_ Exactly that isn't possible exposing a non const reference! – πάντα ῥεῖ Mar 03 '17 at 20:06
  • @πάνταῥεῖ:: (1) Why not? `{int& integer(){ if (life_is_good()) { return integer_; } else throw std::runtime_error("Life's not good!"); }` (2) I suggested a few other uses in addition to that one. – einpoklum Mar 03 '17 at 20:10
  • @ein Smells like poor design, but last resort to keep _dangling_ access off. – πάντα ῥεῖ Mar 03 '17 at 20:13
  • @πάνταῥεῖ: (1) You're right, and I'll edit my answer to make that comment. (2) "No reason to do it" and "Doing it because of a questionable design choice" are two different things. (3) OP asked why are people writing these getters, and this is one of the possible reasons. – einpoklum Mar 03 '17 at 20:16
5

I would strongly discouraged returning a non-const reference to a private variable. Not because it breaks encapsulation, but because it is unnecessary: Why not make the variable public in the first place?

Breaking encapsulation is bad, yes, but that does not mean that every variable should be private. Some variables are meant to be read and modified from the user, so it would make sense to make them public. For example, take std::pair, it has 2 public member variables, first and second. That's not bad practice.

The only time it doesn't make sense is when the variable is not supposed to be written to. That would be bad, as it would actually break encapsulation and make the whole program hard to debug.

sergiol
  • 4,122
  • 4
  • 47
  • 81
Rakete1111
  • 47,013
  • 16
  • 123
  • 162
  • 1
    std::pair doesn't have any behaviour though - if your class has behaviour, then public data is fatal –  Mar 03 '17 at 19:45
  • @NeilButterworth Why? What's the difference? – Rakete1111 Mar 03 '17 at 19:45
  • Exactly as @NeilButterworth said, `pair` has no behavior. It is just container for data. So this is the best way of doing stuff? Having containers different from actors? Other than the case of when you need `friend` – Everyone Mar 03 '17 at 19:47
  • Well, if you have a class with behaviour, such as std::string, what happens if you can gaily change the variables implementing the class? What happens with std::string if you directly change the pointer pointing to storage for the string? –  Mar 03 '17 at 19:47
  • @NeilButterworth The pointer is not meant to be modified, so it has to be `private`, yes. I never said that it makes sense for every variable, but sometimes, it does. Maybe you have a `Window` class where the `width` and the `height` are public, to be modified and read. Then, the next time the main loop reads the width and height, and modifies the window accordingly – Rakete1111 Mar 03 '17 at 19:49
  • Why would you want the "main loop" to have to read these variables every time? In fact for a window class you not only want them to be private, but you want a SetSize( int x, int y ) function so that you don't need to do two updates of the GUI representation. –  Mar 03 '17 at 19:52
  • Hmm, @NeilButterworth `std::string` would not be the best example because it is a class with a lot of behavior. Yet you can still change characters from its internal character array, or change its internal variables from the outside – Everyone Mar 03 '17 at 19:52
  • @Everyone "or change its internal variables from the outside" but not directly, which is the point of all this. –  Mar 03 '17 at 19:53
  • @Rakete1111 Conversely, if `Window` is more complex and needs to perform certain actions, such as managing its own buffer, you would want to disallow direct access to `width` and `height`, so you can force the user to call a member function that guarantees that all necessary actions will be done. That way, you can keep someone from accidentally increasing the window size without expanding the buffer, for example. – Justin Time - Reinstate Monica Mar 03 '17 at 19:54
  • @NeilButterworth Yeah you are basically adding an `operator` that tells the class to access that particular `char` from the internal character array. Some people go really far when describing encapsulation and suggest that this is also not the best way, where they would send me to a point that i have no idea how are you supposed to do anything >. – Everyone Mar 03 '17 at 19:58
  • @NeilButterworth Yeah, that would be better, you're right. But still, there may be some scenarios where a public variable is better, because someone has to have read and write access to it. – Rakete1111 Mar 03 '17 at 19:58
  • Just wanted to draw your attention to my own answer below, since I'm arguing with yours somewhat. – einpoklum Mar 03 '17 at 20:02
  • Isn't it missing **non-const** on *I would strongly discouraged returning a **non-const** reference to a private variable*? – sergiol Mar 21 '18 at 18:28
  • @sergiol Jup. Good catch! Thanks – Rakete1111 Mar 21 '18 at 19:11
3

This construction may be used for debugging purposes.

If you have a public variable, you can't monitor its usage easily. Converting it to a pair of private variable and method-returning reference will allow you to put a breakpoint and/or log the calls.

However, separate getter and setter would serve the same purpose even better, so this is just an advantage over plain public variables.

IMil
  • 1,391
  • 13
  • 17
2

I wrote this once. I planned later to go back and replace the field getter with something that returned a stack object to something that could be cast to the original type and assigned to by the original type. This allowed me to go back later and intercept all assignments to put validations in.

Kinda overpowered techinque. None of the other coders on the project could understand it at all. The whole stack got ripped out.

Joshua
  • 40,822
  • 8
  • 72
  • 132