• Advertisement
Sign in to follow this  

What is this called? And is it a good idea?

This topic is 4401 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

When writing OOP-style code you usually don't let things outside of the class mess with the inside a la:

class cBla
 {
  public:
  int m_some_var;
 }; // end class cBla


Instead you write get- and set-functions:

class cBla
 {
  private:
  int m_some_var;
  public:
  int  m_get_var(void) { ... }
  void m_set_var(int a_var) { ... }
 }; // end class cBla

// You get the idea


And I've got tired of writing get- and set-functions so I did:

// sg_template.hpp (Set and Get template interface)

template <class Type>
class iSGEntity
{
    public:
    virtual Type m_get(void) = 0;
    virtual void m_set(Type a_stuff) = 0;
}; // end class SGEntity



I then have a standard implementation that I normally use:

// ssg_template.hpp (Standard Set and Get template methods)

#include "sg_template.hpp"

template <class Type>
class SSGEntity: public iSGEntity<Type> // Standard Set Get Entity
{
    protected:
    Type m_stuff;
    public:
    virtual Type m_get(void) { return m_stuff; }
    virtual void m_set(Type a_stuff) { m_stuff = a_stuff; }
};


then I might do:

class cBlaha
 {
  public:
  iSGEntity<int> *m_something;
  cBlaha()
   { m_something = new SSGEntity<int>; }
 }


and another time I might do something weird, like put the variable in some special cache or something. Then I simply write another implementation of iSGEntity and use it. So might two questions are: What is this called and is it a good idea? Should I instead use IDE macros to create the SG-methods? Any other thoughts?

Share this post


Link to post
Share on other sites
Advertisement
In short: don't do it.

In long: Firstly, get/set pairs strongly suggest you're doing something wrong. You've also made your code unneccessarily complex. There's a completely superfluous level of indirection that you now have to keep track of, with the only benefit being you've saved yourself from writing two whole lines of code. Additionally, by definition all SSGEntity<T> members have no additional functionality beyond just setting and retrieving values directly. So they are a public variable that is hard to read. You've limited yourself to a single interface (T get() and void set(T)), which will make more complex cases more difficult to write than they should be, completely negating the benefit of not writing those simple cases. And its ugly.

CM

Share this post


Link to post
Share on other sites
No, definitely not a good idea.

What use is an SSGEntity whose only capability is getting and setting a single value (of uncertain meaning). You might be setting a velocity or a color...what if you want to set both? Why is SSGEntity useful?

-Alex

Share this post


Link to post
Share on other sites
Quote:
Original post by Conner McCloud
In short: don't do it.

In long: Firstly, get/set pairs strongly suggest you're doing something wrong. You've also made your code unneccessarily complex. There's a completely superfluous level of indirection that you now have to keep track of, with the only benefit being you've saved yourself from writing two whole lines of code. Additionally, by definition all SSGEntity<T> members have no additional functionality beyond just setting and retrieving values directly. So they are a public variable that is hard to read. You've limited yourself to a single interface (T get() and void set(T)), which will make more complex cases more difficult to write than they should be, completely negating the benefit of not writing those simple cases. And its ugly.

CM


If you provide both get and set methods, just make the variable public, or at least make only one function T& get()

The rules of encapsulation say you shouldn't have any public member variables. People try to adhere to this by making get/set methods. Both are missing the point.

I say, encapsulation is where the user of code only needs to know the interface, not the internals. So in my reading, there isn't any problem at all with public member variables, because where does it say "interface" means "function"?

The benefit of making those variables private is far outweighed by the benefit of having clear and consise code. The only reason get/set methods might be acceptable are when there are extra steps that need to be taken (like locking a mutex) But even that is a weak argument for get/set functions.

Share this post


Link to post
Share on other sites
Quote:
Original post by etothex
Quote:
Original post by Conner McCloud
In short: don't do it.

In long: Firstly, get/set pairs strongly suggest you're doing something wrong. You've also made your code unneccessarily complex. There's a completely superfluous level of indirection that you now have to keep track of, with the only benefit being you've saved yourself from writing two whole lines of code. Additionally, by definition all SSGEntity<T> members have no additional functionality beyond just setting and retrieving values directly. So they are a public variable that is hard to read. You've limited yourself to a single interface (T get() and void set(T)), which will make more complex cases more difficult to write than they should be, completely negating the benefit of not writing those simple cases. And its ugly.

CM


If you provide both get and set methods, just make the variable public, or at least make only one function T& get()

The rules of encapsulation say you shouldn't have any public member variables. People try to adhere to this by making get/set methods. Both are missing the point.

I say, encapsulation is where the user of code only needs to know the interface, not the internals. So in my reading, there isn't any problem at all with public member variables, because where does it say "interface" means "function"?

The benefit of making those variables private is far outweighed by the benefit of having clear and consise code. The only reason get/set methods might be acceptable are when there are extra steps that need to be taken (like locking a mutex) But even that is a weak argument for get/set functions.


These are unrelated issues. The point of encapsulation is to hide implementation details. You want to be able to completely gut the internals of any a class and replace them with something completely new without having to modify the public interface.

The point of acessors/mutators is to allow side effects (either currently or in the future) to accessing/modifying what would otherwise be public data.

Accessors/mutators can break encapsulation if they expose the internal representation of a class to the client.

* Fine from an encapsulation stand point. I would say too many of them would be indicative of poor design, but "too many" is completely relative.

Share this post


Link to post
Share on other sites
Well, in practically all cases of use of getters and setters that only get and set value (but do nothing else), them is used only because "this way i place variable that is de-facto public into 'private' and so i can pretend that i'm are encapsulating something".
The OP example does it in really purely useless form.
In fact there there is more useless way of doing it:
int &get_some_var(){return m_some_var};
This one can not do anything useful _at all_. That's because from get_some_var() you only know that user is going to do something with m_some_var, but can not do anything about it. Well, you can create something on first use, but that's all you can do.

It is IMO better idea to initially have variable in public.
Then, if you need to do something special when variable is set or retrieved, move to private and add getter&setter. So when fixing compiler errors you are going to get forced review of code accessing the variable and have better chances to catch errors or inefficiencies that can be caused by how getter/setter works now.

Example: bone used matrices and had get_matrix() . Some code called get_matrix() 16 times to get individual values, and it was bad but not too bad if compiler optimizes this out. Now bone stores orientation in quaternions only and get_matrix is slow so you will really want to change code to call get_matrix() and set_matrix() minimal number of times.
Another example: now you need to lock mutex prior to changing variable. So, it's is now better to minimize number of calls to get and set.

By the way, instead of get_blabla and set_blabla i mostly use Blabla() and BlaBla(value) . IMHO, it is as clear if Blabla is descriptive, like Name() and Name(value) , and less ugly.

[Edited by - Dmytry on December 30, 2005 7:26:08 AM]

Share this post


Link to post
Share on other sites
i never use get/set things by default. my classes mainly respond to commands. i have queries, but they rarely return the value of an actual variable inside the class( with the notable exception of bools ):

e.g. my current Image class's header file.

class Image{
private:
static std::vector<Image*> loadedImages;
GLuint tex_id;
int width, height;
bool valid;
std::string file_name;
void select();
protected:
void generateTexture( SDL_Surface* );
public:
Image();
virtual ~Image();

bool isValid();

virtual void draw( const Vector& position, float rotation );
virtual void load( const std::string &file_name );
virtual void release();

static void reload( Image* );
static void reloadAllImages();
};

Share this post


Link to post
Share on other sites
Quote:
Original post by Dmytry
Well, in practically all cases of use of getters and setters that only get and set value (but do nothing else), them is used only because "this way i place variable that is de-facto public into 'private' and so i can pretend that i'm are encapsulating something".


The use of accessors/mutators has nothing to do with encapsulation. They can break encapsulation if used sloppily. Public data doesn't even necessarily violate encapsulation though it's bad for other reasons (reasons accessors/mutators and properties exist).

Quote:
Original post by Dmytry
If you change code so getting value and setting value is going to have side-effects, it is IMO better idea to initially have variable in public and then when needed move to private and add getter&setter. So when fixing compiler errors you get forced review of code accessing the variable and have better chances to catch errors or inefficiencies that can be caused by side effects of getter/setter.


The moment someone else begins relying on that interface, that kind of change becomes impossible.

Quote:
Original post by rip-off
i never use get/set things by default. my classes mainly respond to commands. i have queries, but they rarely return the value of an actual variable inside the class( with the notable exception of bools ):

e.g. my current Image class's header file.
*** Source Snippet Removed ***


But now you have violated the one class, one responsibilty principle.

Share this post


Link to post
Share on other sites
Quote:
Original post by wild_pointer
These are unrelated issues. The point of encapsulation is to hide implementation details. You want to be able to completely gut the internals of any a class and replace them with something completely new without having to modify the public interface.

The point of acessors/mutators is to allow side effects (either currently or in the future) to accessing/modifying what would otherwise be public data.

Accessors/mutators can break encapsulation if they expose the internal representation of a class to the client.

* Fine from an encapsulation stand point. I would say too many of them would be indicative of poor design, but "too many" is completely relative.


If you have a class with public member variables then it's part of the interface. It doesn't have to break encapsulation at all.

A bad design will be a bad design regardless of whether it uses get/set functions and private member variables. A good design will be a good design regardless of whether it uses public member variables.

I also don't think that having get/set functions have side effects is good design, but that's not related to encapsulation or OOP design. Rather I think it's bad engineering to "mislead" the user as to what a function does. Perhaps mutex locking (as I said above) is an acceptable side effect. I won't go into a rant here, but I also don't think that even mutex locking counts as "internals".

Share this post


Link to post
Share on other sites
Quote:
Original post by etothex
If you have a class with public member variables then it's part of the interface. It doesn't have to break encapsulation at all.


That's what I said...

Quote:
Original post by etothex
A bad design will be a bad design regardless of whether it uses get/set functions and private member variables. A good design will be a good design regardless of whether it uses public member variables.


For the most part, a good design can't use public member variables (excluding POD types).

Quote:
Original post by etothex
I also don't think that having get/set functions have side effects is good design, but that's not related to encapsulation or OOP design. Rather I think it's bad engineering to "mislead" the user as to what a function does. Perhaps mutex locking (as I said above) is an acceptable side effect. I won't go into a rant here, but I also don't think that even mutex locking counts as "internals".


You can't really mislead the user if the additional instrumentation doesn't concern them. Maybe you want to increment a counter every time an accessor is called, that's none of their business. The most obvious example of something you might want to do in a mutator is some basic sanity or range checking.

Share this post


Link to post
Share on other sites
Quote:
Original post by etothex
I say, encapsulation is where the user of code only needs to know the interface, not the internals. So in my reading, there isn't any problem at all with public member variables, because where does it say "interface" means "function"?


A very good point! I really like this thinking that varaibles can be part of the interface. Just like how you can change the 'time' variable on a microwave directly!

I am now going to go and change a whole lot of my code...

Share this post


Link to post
Share on other sites
Quote:
Original post by Ezbez
Just like how you can change the 'time' variable on a microwave directly!


No, you can't. Try telling your microwave to cook that potato for -5:79.

Share this post


Link to post
Share on other sites
Quote:
Original post by wild_pointer
Quote:
Original post by Dmytry
Well, in practically all cases of use of getters and setters that only get and set value (but do nothing else), them is used only because "this way i place variable that is de-facto public into 'private' and so i can pretend that i'm are encapsulating something".


The use of accessors/mutators has nothing to do with encapsulation. They can break encapsulation if used sloppily. Public data doesn't even necessarily violate encapsulation though it's bad for other reasons (reasons accessors/mutators and properties exist).

my point is that usually get and set is used to achieve nice warm feeling of encapsulation, whereas it practically doesn't encapsulate anything. Like in OP's code.
Or like GetX GetY GetZ SetX SetY SetZ in 3d vector. Totally useless. Doesn't encapsulate anything at all. In case of vector, it isn't bad though as having x,y,z it is part of what we want from 3D vectors... "encapsulating" x,y,z it's like placing dot product and cross product in the private.

Some people learn how to write well-designed code(with encapsulation where necessary and everything), some people learn how to pretend for themselves that they're writing well designed code (with encapsulation and everything), and it is usually achieved by mimicking little details but not large details, or not seeing trees for forest. So they must unlearn what they have learned if they want to really do it. It's same with any other learning.

IMO _ideally_ code "inside" the class should deal with low level concepts and provide higher level concept(s), and code outside should deal with said higher hevel concept(s). The get and set doesn't give much higher level concept than just variable.

[Edited by - Dmytry on December 30, 2005 9:05:51 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by wild_pointer
Quote:
Original post by Ezbez
Just like how you can change the 'time' variable on a microwave directly!


No, you can't. Try telling your microwave to cook that potato for -5:79.

unsigned int cooking_time_in_seconds;

Share this post


Link to post
Share on other sites
Quote:
Original post by Dmytry
Or like GetX GetY GetZ SetX SetY SetZ in 3d vector. Totally useless. Doesn't encapsulate anything at all. In case of vector, it isn't bad though as having x,y,z it is part of what we want from 3D vectors... "encapsulating" x,y,z it's like placing dot product and cross product in the private.


Sure it does. You now have the option of storing the coordinates as 3 ints, 3 floats, an array, a bit vector, etc. Hell, you could use polar coordinates if you wanted.

Quote:
Original post by Dmytry
Quote:
Original post by wild_pointer
Quote:
Original post by Ezbez
Just like how you can change the 'time' variable on a microwave directly!


No, you can't. Try telling your microwave to cook that potato for -5:79.

unsigned int cooking_time_in_seconds;


I don't know of any microwave where you input the cooking times of greater than one minute in seconds. On mine, in fact, you press a number 1-9 and it puts that many minutes up and begins, then you can press a +30 seconds button. Internally it may very well be stored in seconds, the point is that we don't know.

Share this post


Link to post
Share on other sites
Quote:
Original post by wild_pointer
Quote:
Original post by Dmytry
Quote:
Original post by wild_pointer
Quote:
Original post by Ezbez
Just like how you can change the 'time' variable on a microwave directly!


No, you can't. Try telling your microwave to cook that potato for -5:79.

unsigned int cooking_time_in_seconds;


I don't know of any microwave where you input the cooking times of greater than one minute in seconds. On mine, in fact, you press a number 1-9 and it puts that many minutes up and begins, then you can press a +30 seconds button.

Well, with mirowave of course you have complicated display function, and complicated setting interface (buttons). That's because details inside is charges on the memory chip; and to manipulate directly you would need to open chip and put charges on it, say with charged pin and microscope.

But the getter and setter is very different from complicated display interface and complicated manipulation interface. It would be analogous to making microwave that as user interface has two other open chips exposed to outside (for getting and setting using pin and microscope). It is not much better than manipulating with chip directly, the only thing that it can give is possibility to do something when value is modified, say beep when bits is changed, and possibility to replace chip inside with other model but leave the chips outside unchanged.

Share this post


Link to post
Share on other sites
Quote:
Original post by wild_pointer
I don't know of any microwave where you input the cooking times of greater than one minute in seconds. On mine, in fact, you press a number 1-9 and it puts that many minutes up and begins, then you can press a +30 seconds button.

My microwave only inputs seconds, but that's not the point. *If* you are considering replacing the public variable with a handful of naive mutators, then there's a resonably good chance LengthOfTime CookingTime; will work rather well. However, since setting the cooking time is coupled with an action, both methods are probably shitty solutions to the question of how to design a Microwave class.

We can bicker about this all day, but that doesn't address the OP in the slightest. What are people's thoughts on the BBBs scheme?

CM

Share this post


Link to post
Share on other sites
Quote:

hat are people's thoughts on the BBBs scheme?

bad. and pointless. Don't do it. My suggestion for BBB is to unlearn "public members is bad" and learn why public members can be bad and when.

Share this post


Link to post
Share on other sites
I have a few get/set methods around in my code. However none look like this:

void setData(const Data& p_data) {
m_data = p_data;
}

Most getData's look something like this:

void setData(const Data& p_data) {
// assert on data
m_data = p_data;
// send data to hardware
}

roughly the same for getData's:

Data& getData() const {
// assert on m_data
// assert on m_data != hardware data
return m_data;
}

Share this post


Link to post
Share on other sites
Quote:
Original post by wild_pointer
Quote:
Original post by Dmytry
Or like GetX GetY GetZ SetX SetY SetZ in 3d vector. Totally useless. Doesn't encapsulate anything at all. In case of vector, it isn't bad though as having x,y,z it is part of what we want from 3D vectors... "encapsulating" x,y,z it's like placing dot product and cross product in the private.


Sure it does. You now have the option of storing the coordinates as 3 ints, 3 floats, an array, a bit vector, etc. Hell, you could use polar coordinates if you wanted.

Changing your internal representation to int or float or a bit vector all fundementally change the behavior of your class. All you've done by hiding them behind mutators is allow the resulting bugs to compile. Storing them as an array is rather pointless, and if need be it is possible to create public x,y,and z variables that reference the array anyhow [I believe snk_kid developed the strategy].

I agree that the polar coordinates point is a valid one, but trust me when I say you've already lost that argument.

CM

Share this post


Link to post
Share on other sites
Quote:
Original post by Conner McCloud
We can bicker about this all day, but that doesn't address the OP in the slightest. What are people's thoughts on the BBBs scheme?

CM


It's too much extra work for crippled emulation of properties.

[Edited by - wild_pointer on December 30, 2005 10:45:21 AM]

Share this post


Link to post
Share on other sites
emulation of properties that doesn't emulate properties. Because he can not make these properties do anything useful like (for example) update something else on set.

As in

...
void SetSelection(int a){
if(a==m_selected)return;
if(m_selected>=0)m_items[m_selected].UnSelect();
m_selected=a;
if(m_selected>=0)m_items[m_selected].Select();
}
...
int GetSelection(){
DEBUG_BLOCK( if(m_selected>=0)MY_ASSERT(m_items[m_selected].Selected()); )
return m_selected;
}






All useful examples of setters is like that - do something when setting and/or getting.
The OP's thing can not do it, that's why it is useless.

and why not make getters and setters in advance if code doesn't do things like that: IMO if you have m_selected and want to add such additional functionality that were previously absent, it's better if you had it as public and move to private so code will not just compile and you get forced review and chance to catch possible problems caused by sideeffects. If you had get and set that were simply modifying m_selected, your code will compile instantly after modification, but with large chance code will be assuming that get and set are not doing anything extra so when get and set begin to do something extra it will break in strange ways.
Of course that is kinda different for libraries used by other people.

[Edited by - Dmytry on December 30, 2005 10:52:25 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Dmytry
emulation of properties that doesn't emulate properties. Because he can not make these properties do anything useful like (for example) update something else on set.


I didn't look very closely or think it through very well but you could conceivably derive from SSGEntity and overload get and set and have some subset of the abilities of properties but yeah, you're right, it sucks.

Quote:
Original post by Conner McCloud
Changing your internal representation to int or float or a bit vector all fundementally change the behavior of your class. All you've done by hiding them behind mutators is allow the resulting bugs to compile. Storing them as an array is rather pointless, and if need be it is possible to create public x,y,and z variables that reference the array anyhow [I believe snk_kid developed the strategy].


They're all admitedly stupid changes, I was just arguing accessors/mutators aren't completely pointless even when used trivially. I was only thinking you could go from storing x, y, z as ints to floats and just cast in the accessors/mutators, same goes for bitvectors as long as long as it was big enough to store the same range of values.

Personally, I'm happy with my vectors being little more than POD types.

Share this post


Link to post
Share on other sites
no, you're happy with thinking that your vectors are bit more than PODs . Whereas chances are you are either directly sending them to openGL or otherwise use direct access to specific memory layout, or your code copies all data when it needs to send it to anything and is inefficient for no reason.

Share this post


Link to post
Share on other sites
Quote:
Original post by Dmytry
no, you're happy with thinking that your vectors are bit more than PODs . Whereas chances are you are either directly sending them to openGL or otherwise use direct access to specific memory layout, or your code copies all data when it needs to send it to anything and is inefficient for no reason.


I think you misunderstood me. I was just admitting that my vector implementations do in fact use public member variables, it would be a POD type except I believe having a constructor exempts it.

Anyways, thanks for the discussion.

Share this post


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

  • Advertisement