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

292

Author

December 30, 2005 12:11 AM

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?

Conner McCloud

1,135

December 30, 2005 12:26 AM

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

cypherx

204

December 30, 2005 12:59 AM

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

etothex

728

December 30, 2005 04:13 AM

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.

wild_pointer

291

December 30, 2005 06:49 AM

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.

[size=2]

Dmytry

1,151

December 30, 2005 07:26 AM

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]

rip-off

11,000

December 30, 2005 07:44 AM

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();};

blog | twitter

wild_pointer

291

December 30, 2005 08:07 AM

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.

[size=2]

etothex

728

December 30, 2005 08:23 AM

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".

wild_pointer

291

December 30, 2005 08:36 AM

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.

[size=2]

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

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

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

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines