• Advertisement
Sign in to follow this  

Non private privacy

This topic is 4214 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

This is probably a really bad question, but I occasionally have parts of code that look exactly like this. And every time I write it, I'm wondering why I bother.
class Car
{
private:
   int Color;
public:
   const int GetColor() const { return Color; }
   void SetColor(int c) { Color = c; }
};
Would it not be easier to just
class Car
{
private:
...
public:
  int Color;
}
I know some members need an interface for validation, or conversions, or may not even need to be writable at all. But is that why I pretend to make things like this Color variable private, even though it really isn't? To be consistent? So that I don't have 10 private member variables and 4 public? Even though the ton of pointless interface functions makes my class even harder to write, manage, and understand?

Share this post


Link to post
Share on other sites
Advertisement
To my opinion, the top code is a waste of time. But I would like it if member variables could have a public read and a private write.

Share this post


Link to post
Share on other sites
From now on I make variables with both a get() and a set() public. I had code that looked like (before, that is):

ship.setx(ship.getx() + ship.getxspeed());
if (ship.getx() > SCREEN_W)
ship.setxspeed(ship.getxspeed() * -1);


Now it's:

ship.x += ship.xvel;
if (ship.x > SCREEN_W)
ship.xvel *= -1;


Note how if you make it public you can make use of operators such as += and *=, which is impossible with gets and sets.

Share this post


Link to post
Share on other sites
Quote:
Original post by agi_shi
Note how if you make it public you can make use of operators such as += and *=, which is impossible with gets and sets.


You can use the += and *= operators with getters and setters if you pass by reference, but that kind of kills the purpose of making the variable private. Sometimes it makes sense though, especially when getters and setters have more code then just return and set.


#include <iostream>
using namespace std;

class Int
{
int i;
public:
void set(int l){i = l;}
int& get(){return i;}
};

int main()
{
Int i;
i.set(1);
i.get() += 1;
cout << i.get() << endl;
}

Share this post


Link to post
Share on other sites
Quote:
Original post by F-Kop
But I would like it if member variables could have a public read and a private write.

You can accomplish this if you pass structured objects as a const reference.
const CarData &data = Car.GetData();
int test = data.WheelSize; // this is okay
data.WheelSize = 1; // this isn't

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
the whole point of making data private is encapsulation.

So if something is buggy, and you know it is to do with a certain variable, then the problem must be somewhere inside that class, or through its public interface.

I make extensive use of the const keyword in my engine to manage what kind of access each component has to other components.

Eg.

class Foo
{
private:
int bar;
public:
bool SetBar( int );
int ReadBar( void ) const;
int& AccessBar( );
}

if you only have const access to the class (Eg. const Foo& = GetFoo() ) then you can only call ReadBar to copy the value, but if you have full access you can use SetBar or get full access to the actual variable by:
int& bar = myFoo.AccessBar();
bar += 10;
etc...


When i started off writing C++ I never used private, as I thought it was a waste of time. Everything was public, and maintanence was a nightmare.

Now that im writing professional game engine I cant live without the private and const keywords.

I really reccomend you read "Effective C++ 3rd edition" if you want to be a pro C++ coder. I thought I was 'good' at C++ untill I read this, but it explains the reasons why you should do lots of things (like using private).

Share this post


Link to post
Share on other sites
Quote:


So if something is buggy, and you know it is to do with a certain variable, then the problem must be somewhere inside that class, or through its public interface.

This only applies if there are constraints on the variable, e.g. the valid values for it are restricted even more than the type of the variable. If the variable is just an integer and any value is valid, this doesn't matter anymore since it can never become "buggy."

The reason to wrap variables (which don't even have to be private) with get/set functions is usually either to 1) enforce constraints on the variable or 2) allow for possible change, e.g. if you decide to remove the variable all together and instead retreave it dynamically from a file. The first is simply a biproduct of C++'s rather featureless type system, so you're generally forced to use it if you want to maintain an invariant. The latter really depends on your situation and how likely it is that you'll need to change it. If it will never change, or change is very unlikely, making them public is fine--after all, when was the last time you decided to retreave the x and y values of a rectangle from a file or over a network every time you access it ;)

Share this post


Link to post
Share on other sites
I have a class X and a class Y

class X
{
public:
int m_someVar;
};

class Y
{
public:
int GetAVar() const { return m_someVar; }
void SetAVar(int someVar) { m_someVar = someVar; }
};


These classes are used commonly throughout my code. Later on, I decide I want to log every time the value of m_someVar changes, or I decide that I'd like to do a lazy evaluation on m_someVar. For clients of class Y, I just change the implementation of GetAVar and SetAVar and recompile. Clients of X are stuffed.

Read Scott Meyers "More Effective C++" (Item: program in the future tense). Write your code so that it can be extended.

Share this post


Link to post
Share on other sites
Quote:
Original post by agi_shi
From now on I make variables with both a get() and a set() public. I had code that looked like (before, that is):
*** Source Snippet Removed ***
Now it's:
*** Source Snippet Removed ***
Note how if you make it public you can make use of operators such as += and *=, which is impossible with gets and sets.


That's an improvement, but it *should* be more like:


void Ship::move(int left, int right) {
x += xvel;
if (x < left || x > right) { xvel *= -1; }
// FIXME: also clip the position. Possibly cut the motion for the 'critical'
// frame into the normal part and reflected part, and work out a more
// accurate position?
}

ship.move(0, SCREEN_W);

Share this post


Link to post
Share on other sites
One of the reasons to use Sets and Gets also is to make sure the data is valid. Say you are doing some graphic's work. You have a Class for Pixil. In this class you have an X and a Y members. In your set you make sure that the X and Y are within the bounds of the screen. Now being extra paranoid when you do a get you test to make sure the value is within the bounds, if not you toss an assert. This protects your data at all times. Anyone that is using your class will know that it will always give valid data and will give errors when you try to use it wrong. This is writing roboast code. If more people would do this there would be a lot fewer bugs in programs.

theTroll

Share this post


Link to post
Share on other sites
Don't get me wrong. I completely understand the reasons to use Get/Set and private members. But I'm saying there are times when I know the data is safe to do anything with, such as a generic bool value. How badly screwed can a program become because of the value of a bool member?

On the subject of Get/Set, there is also the reference return interface as an alternative. A nice side effect is the smaller number of function names if you rely on intellisense a lot:
class X
{
private:

int Color;

public:

int& GetColor() { return Color; }
const int& GetColor() const { return Color; }
};






This is one of my personal favorite things to be unhappy about:
void DoBlast(const int *data);

class X
{
private:

int *Data;
int Offset;
int MagicNumber;

public:
// Only allowed to non-const owners
int* GetData() { return Data + Offset + MagicNumber * [more calculations]; }

// Allowed to everyone
void BlastData() const { DoBlast( GetData() ); }
};






Kind of a bad example, but hopefully my point made it through alive. It would be nice if the system realized the obvious const use of *data in DoBlast. But instead I have to write two identical versions of GetData(), and add the const keyword to one. And that ruins my plan of only allowing non-const owners to use the function. Is there any other way around this type of thing? A few of my classes have several functions written twice. They aren't big functions, but it's still pretty annoying.

edit: I realize I could have written the GetData code into BlastData's DoBlast, and that would still allow only non-const access to GetData(). Again, bad example.

It would be nice if functions like GetData() could at least be made to take on a role of both const and non-const. Since the code doesn't change at all.

Share this post


Link to post
Share on other sites
Quote:
Original post by Kest
On the subject of Get/Set, there is also the reference return interface as an alternative.


Which I must point out completely eliminates the entire purpouse of making it a private variable in the first place. You can't enforce any invariants, you can't attach actions to the actual modification of the variable, you can't even make it a temp variable, since that'd cause a dangling reference onto the stack.

If you're going to do accessors, do them right. Does your Car class directly represent an instance on the screen? Go ahead and make them full blown accessors (Color and ChangeColor) - in case you want to make the color selection part of the display list you pass off to the renderer, this allows you to attach throwing away the old list to that action.

There are cases when accessors are inappropriate, usually when dealing with "just plain data". Maybe the Car class is simply a record holder in a valet parking system. Attaching actions to the record itself isn't appropriate, as it makes reusing it in both ParkedCarLocator and CrashLawsuitTracker harder. Maybe Car does not handle rendering management, simply describing an instance of a car within a simulation, and SceneManager handles transforming such vital statistics as color into an appropriate (possibly cached) display list.

It depends on what the responsibility of the class covers, in the end.

Share this post


Link to post
Share on other sites
Quote:
Original post by agi_shi
From now on I make variables with both a get() and a set() public. I had code that looked like (before, that is):
*** Source Snippet Removed ***
Now it's:
*** Source Snippet Removed ***
Note how if you make it public you can make use of operators such as += and *=, which is impossible with gets and sets.


Your example is not very good, because in your case you can greatly improve encapsulation using methods such as move() and maybe changeDirection() - although you can live with only a move() method. The internal code of move() might be set(speedx+pos), but your caller don't have to know this. What if you decide to add some drag support to your code? You'll have to modify every call of setx() to reflect the change, which will obviously be tricky.

Encapsulation of data + direct accessors is a rather bad idea in most situation. In fact, what you should do is not encapsulation of your data but encapsulation of your object behavior, which is quite different. Ideally, once your class is initialized, the only accessor you'll need is a get accessor. Try this: remove all your set() accessors - every modification of the data is then achieved through other ways. You'll see that this simple thing (which admitedly seems to be a nightmare when you look at your code which is full of set() calls) will greatly improve the quality of your code. In fact, this will allow you to achieve one of OO "goal" (can't find a better word now - I'm still tired): program to an interface. This sentence doesn't mean that you program using the object interface. It means that you program using objects which represent a state, and the exact object state is meaningless when you use it.

Encapsulation is the first thing to understand if you want to be serious about OOD. Correct encapsulation leads to many OO principles (for example, neither OCP nor LSP can't be achieved if you need to worry about the internal state of your object).

set() accessors are often abused. Their only goal is to reset a value - and this operation is very rare since most of the time you use the value to perform some operation. To simplify, let's say that set() is teh Evil.

Of course, since set() is teh Evil, public variables are even worse. Public variables means that the object is not able to constraint the variable anymore and can't rely on their state. In turn, it means that any method which access to these variables is useless since it can't be sure that it operates on a meaningful object state.

Quote:
Original post by Kest
Even though the ton of pointless interface functions makes my class even harder to write, manage, and understand?

One of the most important thing about OO programming with C++ is: don't be afraid of creating a large number of small methods.

Share this post


Link to post
Share on other sites
Quote:
Original post by bytecoder
The reason to wrap variables (which don't even have to be private) with get/set functions is usually either to 1) enforce constraints on the variable or 2) allow for possible change, e.g. if you decide to remove the variable all together and instead retreave it dynamically from a file. The first is simply a biproduct of C++'s rather featureless type system, so you're generally forced to use it if you want to maintain an invariant. The latter really depends on your situation and how likely it is that you'll need to change it. If it will never change, or change is very unlikely, making them public is fine--after all, when was the last time you decided to retreave the x and y values of a rectangle from a file or over a network every time you access it ;)

Well, there's one other reason, a mix of #1 and 2, I guess. What if you want to add constraints later on? Then it's nice to already have the get/set functions set up, so you don't have to dig through your code and replace every use of the public variable with a call to the get/set function. :)

Share this post


Link to post
Share on other sites
Quote:
Original post by Kest
Don't get me wrong. I completely understand the reasons to use Get/Set and private members. But I'm saying there are times when I know the data is safe to do anything with, such as a generic bool value. How badly screwed can a program become because of the value of a bool member?

Very badly screwed. If you one day find out it shouldn't have been a simple bool after all, but maybe an enum, or maybe a set of flags (This goes back to what Emmanuel Deloget said. Program to an interface, so external code doesn't know the exact implementation of the class in the first place. Don't have a SetBool function. Use one that manipulates the *state* of the object as a whole. If your bool represents whether your spaceship is docked at a space station, make a Dock() function instead. Then the internal representation no longer matters)
Maybe your bool should also enforce some kind of variant (only set to false if a number of other conditions are set, or if you try to set it to true, a bunch of other changes should also be made to other member variables)

Can you be absolutely 100% sure that you'll never need to change *anything else* when you change the bool variable? That changing the bool without doing anything else can never cause problems?

Share this post


Link to post
Share on other sites
It's not wrong to have public data. You just have to remember one thing: Making data public moves it out of the data realm and into the interface. Public data is just as much part of the interface to a class as a public member function is. (For that matter, non-member functions can be part of it's interface too...)

So, as long as you understand that idea, you won't feel nervous about public data - after all, you'll only use it when it is logically part of the interface.

Share this post


Link to post
Share on other sites
Quote:
Original post by Emmanuel Deloget
Encapsulation of data + direct accessors is a rather bad idea in most situation. In fact, what you should do is not encapsulation of your data but encapsulation of your object behavior, which is quite different. Ideally, once your class is initialized, the only accessor you'll need is a get accessor. Try this: remove all your set() accessors - every modification of the data is then achieved through other ways. You'll see that this simple thing (which admitedly seems to be a nightmare when you look at your code which is full of set() calls) will greatly improve the quality of your code. In fact, this will allow you to achieve one of OO "goal" (can't find a better word now - I'm still tired): program to an interface. This sentence doesn't mean that you program using the object interface. It means that you program using objects which represent a state, and the exact object state is meaningless when you use it.

I really understand where you're coming from, but sometimes, a lot of times, you just need to tell an object to modify one attribute. What happens when you just need to paint the car quickly and efficiently? Nothing is required other than changing the color. You could try to allow the car to handle the request to paint itself, which would work well with scripting, but not with much else.

I realize that bettering the car interface would be easy using methods like GetPaintColor() and Repaint(). But with most object types, no change is taking place here other than renaming the functions. So the result is still the same.

Quote:
Original post by Spoonbender
Don't have a SetBool function. Use one that manipulates the *state* of the object as a whole. If your bool represents whether your spaceship is docked at a space station, make a Dock() function instead. Then the internal representation no longer matters).

and
Quote:
Original post by Emmanuel Deloget
Encapsulation is the first thing to understand if you want to be serious about OOD. Correct encapsulation leads to many OO principles (for example, neither OCP nor LSP can't be achieved if you need to worry about the internal state of your object).

The color of the car is not very internal. But again, I totally follow you and completely agree. It's just that many external states are stand-alone states. The game.. err program.. needs to be capable of updating or accessing them on the fly to work correctly.

The car may not be the best example. So what about the foreground color of a GUI interface control? How else could you organize the setting and getting of colors on a GUI, other than Set/GetColor()? What if you need to change it often, regardless of what the GUI itself is doing? In other words, events are taking place that are external to it's operation. This happens a lot more in gaming than in application programming.

Quote:
Original post by Emmanuel Deloget
set() accessors are often abused. Their only goal is to reset a value - and this operation is very rare since most of the time you use the value to perform some operation. To simplify, let's say that set() is teh Evil.

What's a really big pain in the ass is modifying objects from an editor. I've done everything I can to avoid set functions for every attribute in some objects. Made the objects use structures to hold all of their attributes so the editor can send a single update to them, or created brand new objects and deleted old ones just to update one member variable. I've yet to find anything that I'm happy with.

Quote:
Original post by Deyja
It's not wrong to have public data. You just have to remember one thing: Making data public moves it out of the data realm and into the interface. Public data is just as much part of the interface to a class as a public member function is. (For that matter, non-member functions can be part of it's interface too...)

So, as long as you understand that idea, you won't feel nervous about public data - after all, you'll only use it when it is logically part of the interface.

I like the way you make it sound, but when exactly is it logical? I don't have any classes at all that have private and public data. And my project is composed of 168 source files (src and headers), more than 200 classes, and 300 structures.

Share this post


Link to post
Share on other sites
Quote:
Original post by Daniel Miller
Even if you don't do any data checking or anything, you can still put breakpoints in accessors.

You can also break when variables are modified.

Share this post


Link to post
Share on other sites
Quote:

I realize that bettering the car interface would be easy using methods like GetPaintColor() and Repaint(). But with most object types, no change is taking place here other than renaming the functions. So the result is still the same.


That's right. The solution is to abandon setters completely. What's bad in setters is that you can change the state of the object anywhere in the code. You don't want that. Let's say, for example, that you need set/get so you can change the color of the car based on keyboard input: With VK_UP, you increase the color, with VK_DOWN, you decrease it. First problem is that if you later want to change that behaviour, you have to hunt them down throughout the code and make the changes. A more important problem(especially in graphics engine) is this:


UpdateAllObjects();
DoStuff();
SortAllObjectsByColor();
DoMoreStuff();
DrawAllObjects();





Now, if you call SetColor/Repaint/whatever inside DoMoreStuff(), you'll have incorrect behaviour. You will have to lay down, memorize and uphold all kind of little rules like "never change the color of an object between material sorting and rendering".

I think a better approach would be something like:


typedef int Color;
class Car
{
private:
Color m_color;
protected:
virtual Color UpdateColor(Color color)=0;
public:
Car(Color color):m_color(color){}
void Update()
{
//DoStuff();
m_color=UpdateColor(m_color);
//DoOtherStuff();
}
};

class CarImpl:public Car
{
protected:
virtual Color UpdateColor(Color color)
{
if (key[VK_UP]) return color+0.01;else
if (key[VK_DOWN]) return color-0.01;else
return color;
}
public:
CarImpl(Color color):Car(color){}
};





Now you have a clear point where only there the color of the car is allowed to change, so you can define there the behaviour you want without using set/get at all.

Share this post


Link to post
Share on other sites
What if your input/event data haseth private data, rather than easy-to-work-with key[VK_..] globals? You would still need to use some GetThis and GetThats in that UpdateColor() function. Also, some people, most of which reside in this forum, would yell at anyone trying to mess with keyboard data in a car class ;)

Share this post


Link to post
Share on other sites
Quote:
Original post by Kest
What if your input/event data haseth private data, rather than easy-to-work-with key[VK_..] globals? You would still need to use some GetThis and GetThats in that UpdateColor() function. Also, some people, most of which reside in this forum, would yell at anyone trying to mess with keyboard data in a car class ;)


Uhm, the above was just a quick example to make my point. I didn't say you would actually handle keyboard input like that. The point is that the car class is the one that requests from a "painter" module to alter its color.

Share this post


Link to post
Share on other sites
Quote:
Original post by Kest
The color of the car is not very internal. But again, I totally follow you and completely agree. It's just that many external states are stand-alone states.

True. The question is, can you always be sure that they'll *stay* stand-alone states? What if, at some point in the future, you decide that red cars can go faster? Everyone knows that's true anyway. [lol]
Now, if you'd used a setter all along, you could simply modify it to adjust the car's maxspeed variable when the color changes. If you were accessing the color directly, it gets messier.

Yeah, I know, another awfully contrived example...
I guess the point is just that it's usually "better" not to use expose members of a class. Especially by making them public, but setter functions are also typically a sign that something is wrong.
But yes, you're right, that's not *always* the case, and you're right, it *is* sometimes easier to just make the damn thing public... [grin]

As long as you understand the reasons why you *shouldn't* do it, you can always decide when to break that rule. Most rules need to be broken every once in a while. [lol]

Share this post


Link to post
Share on other sites
At any rate, I NEVER write get or set in a function name. If I need that behavior, but still need to hook in behavior when the variable is changed, I will disquise a private member as a public member.


class foo
{
private:
int private_m;
void hook_m_assign();
void hook_m_accessed();
public:
Property<int,hook_m_assign,hook_m_accessed> m;
};

Share this post


Link to post
Share on other sites
Quote:
Original post by mikeman
Quote:
Original post by Kest
What if your input/event data haseth private data, rather than easy-to-work-with key[VK_..] globals? You would still need to use some GetThis and GetThats in that UpdateColor() function. Also, some people, most of which reside in this forum, would yell at anyone trying to mess with keyboard data in a car class ;)


Uhm, the above was just a quick example to make my point. I didn't say you would actually handle keyboard input like that. The point is that the car class is the one that requests from a "painter" module to alter its color.

But your example was to show why Get and Set are not needed. What if your keyboard class, or whatever input or event system you use to update color, has it's own private data? In other words, your virtual Color UpdateColor(Color color) either needs more input params, or needs to access a global. And in either case, you need to use a Get method to see if the event is true. Or in other words, Keyboard->GetKeyValue(VK_DOWN).

Quote:
Original post by Deyja
At any rate, I NEVER write get or set in a function name.

Just avoiding the name doesn't really accomplish anything, does it? What about the example I gave earlier? GUI.SetForegroundColor()? Or GUI.SetFont()? What clever interface names are there for these actions that would make the GUI system feel more like a machine with buttons?

Quote:
If I need that behavior, but still need to hook in behavior when the variable is changed, I will disquise a private member as a public member.

class foo
{
private:
int private_m;
void hook_m_assign();
void hook_m_accessed();
public:
Property<int,hook_m_assign,hook_m_accessed> m;
};

Looks like that's just more work. What is the advantage?

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement