Is returning a pointer to a private member not good?

class Foo
{
  type var_;
public:
  const type& var() const { return var_; }
  Foo& var(const type& value) { var_ = value; return *this; } 
};

---

“Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.” — Brian W. Kernighan (C programming language co-inventor)

Yeah, generally it''s bad to allow access to member data. The accepted practice is to have two functions, GET and SET, that respectively allow you to view and change that data according to the class rules.

int Bunghole::GetCoffeeLevel() { return m_coffee; }
void Bunghole::setCoffeeLevel( int pcoffee )
{ 
  if (pcoffee<=0) { m_coffee=0; }
  else if (pcoffee>=100) { m_coffee=100; }
  else { m_coffee=pcoffee; }
} 

Or, if it''s a lot of string data, either demand that the calling function deliver a pointer to it''s own buffer (for you to copy into) or have a member buffer that you''ll copy the string into and return the pointer to that buffer.

O---k...two questions that really don''t belong here, but I need to feel special.

quote:

---

Original post by Sedul
(snip)

someClass classInstance;

type* pType = classInstance->getVar();
*pType = somevalue;

(SNIP SNIP SNIP)

---

I thought you had to declare a variable (classInstance) as a pointer to use the pointer-to-member function...can you use it normally with it?

quote:

---

Original post by Fruny
class Foo
{
type var_;
public:
const type& var() const { return var_; }
Foo& var(const type& value) { var_ = value; return *this; }
};

---

What is with the const modifier before { return var_; }? Does it affect anything?

Sorry about the n00bish questions, I''m still learning the finer points of the language, as well as learning 2 others at the moment.

"TV IS bad Meatwad...but we f***in need it"

get/set tuples are a code smell; it may as well be public.
It fails to abstract any meaningful concept - it abstracts how to retrieve var? and how to set var? That''s exetremely trivial (too fine a level of granuality for method abstractions).

e.g. ChangeMyIPAddress(ip,mask,gw) ip WhatsMyIPAddress() behave similarly to a get/set tuple, but they tell you what is going on, and actually hide a lot of work (meaningful abstraction).

Don''t put in abstractions that are not required it increases complexity for no gain. Do it when and if it needs it.

Fruny:

So type _var is not a private member? even though it wouldnt'' matter if it''s private or not private.


What if you get that dataacess through the reference returned but the object instance Foo gets destroyed.

Then your variable that references that member var of that class will no longer reference properly?

Yeah I agree, if you''re exposing the address then just make it public.

You have two main options (not counting the more exotic solutions)
1. make it public, the benefit is that you don''t have to write getters/setters
2. make it private with get/set, the benefit is that you can be sure that no one can change the value, unless they do it through set.

If you make it private and expose the address, then you''re getting the worst of both worlds

quote:

---

Original post by Sedul
Fruny:

So type _var is not a private member? even though it wouldnt'' matter if it''s private or not private.


What if you get that dataacess through the reference returned but the object instance Foo gets destroyed.

Then your variable that references that member var of that class will no longer reference properly?

---

To my knowledge all class members in C++ default to private.

In Fruny''s snip he didn''t specify any access type, which, unless it was a typo, did make var_ private.

"TV IS bad Meatwad...but we f***in need it"

Boku san - the const qualifies the member function. It means it doesn't modify any (non-mutable) member variable and can therefore be called even on const objects:

const Foo f;
f.var();       // Ok, Foo::var() is const
f.var(blah);   // Error, f is const   

So type _var is not a private member?

Members of a class are private by default.

What if you get that dataacess through the reference returned but the object instance Foo gets destroyed.

Depends on whether you did store a pointer/reference or made a copy. Returning a reference is better than returning a pointer, since the user of the code has to go out of his way to actually do bad things with them.

Foo* f = new Foo;
type &ref =  f->var();
type *ptr = &f->var();
type  val =  f->var();
delete f;   

ref and *ptr will be invalid, but val will be fine. Returning by reference is a convenience only (may avoid copying and remove the need for a pointer dereference). Returning a const reference avoids accidental modifications.

Then your variable that references that member var of that class will no longer reference properly

You can't bullet-proof against people keeping references (or pointers) to dead objects, short of using smart and weak pointer classes.

---

“Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.” — Brian W. Kernighan (C programming language co-inventor)

[edited by - Fruny on March 10, 2004 10:08:50 PM]

[edited by - Fruny on March 10, 2004 10:10:37 PM]

thx for the quick and informative response. This has been really helpful!

quote:

---

Original post by Sedul
thx for the quick and informative response. This has been really helpful!

---

Pay special attention to Magmai''s reply, and pick up a copy of the book /Refactoring/ by Fowler. It talks about this and other bad code smells.



--
Dave Mikesell Software

quote:

---

Original post by Magmai Kai Holmlor
e.g. ChangeMyIPAddress(ip,mask,gw) ip WhatsMyIPAddress() behave similarly to a get/set tuple, but they tell you what is going on, and actually hide a lot of work (meaningful abstraction).

---

I fail to see how "ChangeMyIPAddress" is more abstract than SetIPAddress. It's just longer to type and more confusing, because the function name is unusual.

Get/set isn't necessarily bad, IMO. I agree that if they are trivial functions (and you're sure that they always will be) then you might as well make the member public. But if you are (or plan on) checking the values, or updating a cache, then get/set can be appropriate.

Cédric

[edited by - Cedric on March 11, 2004 7:51:29 AM]

quote:

---

Original post by Magmai Kai Holmlor
get/set tuples are a code smell; it may as well be public.
It fails to abstract any meaningful concept - it abstracts how to retrieve var? and how to set var? That''s exetremely trivial (too fine a level of granuality for method abstractions).

e.g. ChangeMyIPAddress(ip,mask,gw) ip WhatsMyIPAddress() behave similarly to a get/set tuple, but they tell you what is going on, and actually hide a lot of work (meaningful abstraction).

Don''t put in abstractions that are not required it increases complexity for no gain. Do it when and if it needs it.

---

There are some other advantages to using get/set aside from access to private members. They are very useful for debugging. For instance, if you have a class that is being used all over the place, and it''s getting set to a bad value, just put a print statement in there (or a breakpoint), and you can easily find the statement that is munging your code. People often forget that there are other reasons for having code like this in your program aside from pure functionality/performance.

quote:

---

Original post by Magmai Kai Holmlor
get/set tuples are a code smell; it may as well be public.
It fails to abstract any meaningful concept - it abstracts how to retrieve var? and how to set var? That's exetremely trivial (too fine a level of granuality for method abstractions).

---

I disagree with this. Take for example a rigid body class in physics. It has an mMass variable. In most physics engines, the inverse mass is used in many calculations in the physical simulation and is defined as mInverseMass = 1.0f / mMass.

Now if you made the mMass variable public, when you tried to change the value only mMass would be changed, but for the simulation to work properly mInverseMass would also need to be changed to reflect the new value of mMass. A setter method would be perfect to do this. All you would need to do is call SetMass( 5.0f ) and that function could wrap altering the mMass variable and also recalculate the inverse mass.

The point here is that by using setter methods, the class is 'overseeing' what you are doing and stops you making errors like the one above. If you want to alter the variable mMass, you shouldn't have to worry about also recalculating mInverseMass, the class should do this via its set method.

In conclusion, setter methods are often a safeguard to stop you making mistakes. It doesn't mean you might as well make the data public.



[edited by - gowron67 on March 11, 2004 5:25:12 PM]

(In response to AP above) Bjarne Stroustrup says: "I particularly dislike classes with a lot of get and set functions. That is often an indication that it shouldn't have been a class in the first place. It's just a data structure. And if it really is a data structure, make it a data structure."
Read it here
Who will you believe?

[edited by - amag on March 11, 2004 5:32:00 PM]

From the same article (or, at least, the one you linked to..) :

"If every data can have any value, then it doesn''t make much sense to have a class."

The key is the "if every data can have any value" part. If the members of your class are interrelated in any way whatsoever, you''ll want get() and set() functions to make sure that the correct relationships are preserved. Take, for example, a class with a min, a max, and a value -- using gets and sets, you can make sure that the max isn''t less than the min, and the value is between the max and the min, with only one line of code from the user''s point of view -- SetMin(), SetMax(), or SetValue(). A similar class which uses just public member variables would be a pain, because you''d have to code (and code again, and again, and again) all the validation every time you wanted to change anything.

So, if your "class" really is just a package of variables, each of which can take any of the possible values of its type and none of which bear any relation to any of the others, then get()s and set()s probably aren''t worth the trouble. But, on the other hand, if you''re writing a lot of classes like that, you''re probably not using the full potential of an object-oriented language anyway, and a couple of extraneous get()s and set()s are probably the least of your worries.

Fruny, in your signature you credit Brian Kernighan as a "co-inventor" of C. If I''m not mistaken, this is not correct. Ken Thompson created the language B from which Dennis Ritchie derived C. Brian Kernighan simply was a co-author with Ritchie of the book, The C Programming Language. Perhaps you know something about C''s history that I don''t, so feel free to clarify.

quote:

---

Original post by merlin9x9
Perhaps you know something about C's history that I don't, so feel free to clarify.

---

No, I don't, it's just the way he's presented in my books. If you're sure of your data, I'll fix my sig - no problem.

---

“Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.” — Brian W. Kernighan

[edited by - Fruny on March 11, 2004 8:08:16 PM]

CautionMan, I can also invent situations when get/set is good. It's just that when you do an abstraction, you actually should gain (at least) one layer (after all, abstraction is about simplification). If you don't, then what you've done is useless. Many ppl seem to believe that if they just write get/set-methods then they do OOP, which is not neccessarily the truth.
Actually I agree with most of what you say, except for the "probably" in your last paragraph.

Also to AP above. That's one of the worse arguments for get/set-methods - thinking about the future.
Either
A) Your project is carefully planned and every class/function/etc is carefully designed. Then you have get/set where they make an abstraction.
B) Your project is not so carefully planned in which case you're bound to change interfaces and dependencies between classes/functions/etc frequently enough anyway.

Writing code that deals with the future is humbug!

[edited by - amag on March 11, 2004 8:24:53 PM]

quote:

---

Original post by Cedric

quote:

---

Original post by Magmai Kai Holmlor
e.g. ChangeMyIPAddress(ip,mask,gw) ip WhatsMyIPAddress() behave similarly to a get/set tuple, but they tell you what is going on, and actually hide a lot of work (meaningful abstraction).

---

I fail to see how "ChangeMyIPAddress" is more abstract than SetIPAddress. It''s just longer to type and more confusing, because the function name is unusual.

Get/set isn''t necessarily bad, IMO. I agree that if they are trivial functions (and you''re sure that they always will be) then you might as well make the member public. But if you are (or plan on) checking the values, or updating a cache, then get/set can be appropriate.

---

If Get/SetIpAddress are acceptable, then IpAddress ought to be as well {ip IpAddress(), IpAddress(ip)}. If IpAddress if good, then what you /really/ want is a utility class(es) that spruces up the legacy bsd code - i.e. implicitly convertable to the old type. Data is all public for maximum utility, with added convienence functions (c_str() from_string(...) ). You get context from the variable name then to tell you what address you are fiddling with.

I was trying to impress that I was actually changing the active and bound ip on the host computer (albeit a /good/ design would change it on a device, and allow for secondaries).