Public or Private?
Author
I'm rather new to C++, and doing my first solo program in it. I started off by making the majority of the member variables of my classes protected/private, and then had public functions which allowed access to the member variables when required. As the program grew, I ended up needing two functions for a lot of my member variables (one to set it, and one to read it).
It seems that I've got a poor design. Should I just make the variables public and do away with the accessing functions? Are my classes designed wrong in that other objects need direct access to my member variables so much? I can provide more details if this is too vague. Any insight is welcome.
As a general rule of thumb. If you can get away with keeping objects (classes/structs etc..) as well defined as possible to prevent blurring between them by accessing stuff explicitly as public variables, your better off.
That way your code is easier to maintain and you can drop in replacements for those objects in the event you need to optimize them or expand for more functionality.
Its a good idea to have public functions to access private variables. That way you can drop into the debugger and watch with greater ease as variables are requested or set.
g/l
That way your code is easier to maintain and you can drop in replacements for those objects in the event you need to optimize them or expand for more functionality.
Its a good idea to have public functions to access private variables. That way you can drop into the debugger and watch with greater ease as variables are requested or set.
g/l
Quote:Its a good idea to have public functions to access private variables.
Get/set functions are almost always a sign of a bad design.
Quote:That way you can drop into the debugger and watch with greater ease as variables are requested or set.
WTF?
Quote:Original post by DeyjaQuote:Its a good idea to have public functions to access private variables.
Get/set functions are almost always a sign of a bad design.
It is bad design only if excessively overused. But there is a justification to use them where possible because sometimes you may not just do assignment/retrieval. Using get/set functions permits us to perform additional checking or verification transparent to the user.
Quote:Quote:That way you can drop into the debugger and watch with greater ease as variables are requested or set.
WTF?
I agree with Undergamer on this. Although it is possible to set breakpoint conditions when a variable changes, that is still subject to the debuggers ability and customizability. By using function based assignment/retrieval, you can use (for instance) a general breakpoint within them. So each time the variable is accessed or modified, it's a lot easier to catch it rather than a watch or what-not.
I don't recommend making your class variables public, but you can make life a little easier for yourself if you define some of your getter/setter methods like this instead:
Then you can access the health variable as if it were made public:
The problem with just out-right making your class variables public is that if you then decide to change the name of the variable, you'll have to change it everywhere else in your code. Also, if you want to make the class do something when the health variable is accessed, that goes out the window too by making everything public.
But yes, if you think you'd be better off having all of your variables made public, there's something wrong with your design, you're giving a class too much responsibility.
Perhaps a struct might be better suited to your needs?
class Player{ int m_health;public: Player() : m_health(0) { } int& Health() { return m_health; } const int& Health() const { return m_health; }};Then you can access the health variable as if it were made public:
Player player;player.Health() = 100;int health = player.Health();The problem with just out-right making your class variables public is that if you then decide to change the name of the variable, you'll have to change it everywhere else in your code. Also, if you want to make the class do something when the health variable is accessed, that goes out the window too by making everything public.
But yes, if you think you'd be better off having all of your variables made public, there's something wrong with your design, you're giving a class too much responsibility.
Perhaps a struct might be better suited to your needs?
Quote:Should I just make the variables public and do away with the accessing functions?
Having get/set methods is common practice. In some cases, it is appropriate to make member variables public (if your class is conceptually a basic type. eg, matrix, vector, etc), but in the overwhelming majority of cases it is not. Keep the variables abstracted as you are currently. Hiding the internals of your classes is the most important part of encapsulation.
Quote:Are my classes designed wrong in that other objects need direct access to my member variables so much?
It is a sign of high coupling, which is a symptom of poor design. Your classes will of course have to share data with one another, but ideally, the number of connection points between them should be low. Examine your design: you might be performing calculations on those variables you are accessing that should be internal to the class. If all your classes end up with a lot of get/set methods, and few other public methods, you probably need to redesign your system.
Another thing that came to mind is consistancy.
If you added a function to the player class that would return the amount of damage sustained, you would have to do something like this:
If anyone ever saw your code looking something like this:
They'd probably tell you to watch your back. It doesn't look particularly ugly now, but do that everywhere in your code, and you'd probably cry.
But that's just my opinion.
If you added a function to the player class that would return the amount of damage sustained, you would have to do something like this:
int GetDamage() const{ return 100 - m_health;}If anyone ever saw your code looking something like this:
Player player;player.m_health = 60;int damage = player.GetDamage();They'd probably tell you to watch your back. It doesn't look particularly ugly now, but do that everywhere in your code, and you'd probably cry.
But that's just my opinion.
Quote:I don't recommend making your class variables public, but you can make life a little easier for yourself if you define some of your getter/setter methods like this instead:
...return by reference...
Don't do this. By requiring your class to return a specific variable by reference, you totally limit the changes you can make to this interface later on. Suppose you originally had a member variable of the class which this get method provided access to, but later on you remove it entirely, and generate it somehow. Eg:
class Rectangle{public: int GetWidth() const {return width;} int GetHeight() const {return height;} int GetArea() const {return area;} void SetWidth(const int& awidth) { width = awidth; area = width * height; } void SetHeight(const int& aheight); { height = aheight; area = width * height; }private: int width; int height; int area;};Later on you remove the area member, and within the get method you calculate it from the width and the height. With your method of returning by reference, you can't do this so easily. You have to change the public interface to the get method itself. This is a bit of a simple example, but appreciate the concept.
That is true, however common sense would prevail in cases where you would and wouldn't use it.
I guess since he's new to C++ it'd be bad for me to suggest such things =x
I guess since he's new to C++ it'd be bad for me to suggest such things =x
Author
What I'm working on is a GUI for my OpenGL program. I want to be able to select things within the GUI that affects the program, and have it display information. Here's an example of what I can do with my setup:
This makes it very, very easy to add to and/or modify my GUI. The rendering engine provides pointers to information and settings to the GUI class. So I'm going to have many "Get<engine stuff>Ptr()" functions. Different GUI 'widgets' will take the different information and may be able to directly modify the rendering engine data. Therefore, the GUI widgets need "Set<engine stuff>Ptr". I also have GUI widget copying functions (it allows me to easily and dynamically grow the GUI interface) which require corresponding "Get<>Ptr" functions for the widgets as well. Each widget class is filled with mostly Get/Set pointer functions (most of the generating and drawing is taken care of by their common base class) and member variables specific to that widget, so maybe it's not so far off base.
This is my first shot at something like this, so I'd like suggestions on what a better method might be.
BeginTab("File"); { InsertDivider("Saving and Loading"); InsertDivider("Exit The Program"); BeginButton("Exit"); { LinkToButton(FUNCTION_EXIT, pRenderEngine->GetTerminateProgramPtr()); } EndButton(); } EndTab(); BeginTab("Status"); { BeginTextBox("Frames Per Second:"); { LinkToTextBox(pRenderEngine->GetFramesPerSecondPtr(), TYPE_UINT); SetTextBoxEditable(true); } EndTextBox(); BeginTextBox("Second Per Frame:"); { LinkToTextBox(pRenderEngine->GetSecondsPerFramePtr(), TYPE_FLOAT); SetTextBoxEditable(true); } EndTextBox(); } EndTab(); BeginTab("Camera"); { } EndTab(); BeginTab("Lights"); { InsertDivider("Global Lighting"); BeginCheckBox("Lighting Enabled"); { LinkToCheckBox(pRenderEngine->GetLightingEnabledPtr()); } EndCheckBox(); InsertDivider("Local Lights"); } EndTab(); BeginTab("Objects"); { InsertDivider("Current Object"); BeginTextBox("Object Name"); { LinkToTextBox(pRenderEngine->GetCurrentObjectPtr()->GetNamePtr(), TYPE_STRING); SetTextBoxEditable(true); } EndTextBox(); } EndTab(); BeginTab("Options"); { InsertDivider("Global Drawing Options"); BeginCheckBox("Normals Enabled"); { LinkToCheckBox(pRenderEngine->GetNormalsEnabledPtr()); } EndCheckBox(); } EndTab(); BeginTab("Help"); { InsertDivider("Help Menu"); InsertDivider("About"); } EndTab();This makes it very, very easy to add to and/or modify my GUI. The rendering engine provides pointers to information and settings to the GUI class. So I'm going to have many "Get<engine stuff>Ptr()" functions. Different GUI 'widgets' will take the different information and may be able to directly modify the rendering engine data. Therefore, the GUI widgets need "Set<engine stuff>Ptr". I also have GUI widget copying functions (it allows me to easily and dynamically grow the GUI interface) which require corresponding "Get<>Ptr" functions for the widgets as well. Each widget class is filled with mostly Get/Set pointer functions (most of the generating and drawing is taken care of by their common base class) and member variables specific to that widget, so maybe it's not so far off base.
This is my first shot at something like this, so I'd like suggestions on what a better method might be.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
