• Advertisement
Sign in to follow this  

using enums to generalize set and get functions

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

hi guys, quick question about set and get functions(obviously) for a class with a lot of member values. Instead of making a set/get function for each member, just let the class user send the variable they want in the argument. just wondering what the drawbacks to this method and if it goes against any 'rules'. ps - if this is bad just because it's not good practice...please tell me WHY its not a good practice. please :) thanks in advance! example

enum memberName{member1, member2, member3};

class blah
{
private:
     int member1;
     int member2;
     int member3;
public:
     int getMemberValue(memberName);
}
int blah::getMemberValue(memberName member)
{
     switch(member)
     {
          case 0: return member1; break;
          case 1: return member2; break;
          case 2: return member3; break;
          default: break;
     }
}


pps- forgive any syntax errors that might exist. just trying to get the concept across.

Share this post


Link to post
Share on other sites
Advertisement
Your method reinvents an existing wheel—associative containers. I would probably use an std::map<memberName,int> instead of your class, because it's simpler to write.

Share this post


Link to post
Share on other sites
Quote:
Original post by cwyers
hi guys,
quick question about set and get functions(obviously) for a class with a lot of member values. Instead of making a set/get function for each member, just let the class user send the variable they want in the argument.

What would that gain you?

Share this post


Link to post
Share on other sites
Well personally I think this raises a lot of issues. The first issues is that it is incredibly rigid. What if you had a class that had member variables that were not all integers. Instead it held pointers to other class variables or other built in types how would the getMemberValue function know what type to return? Another issue deals with the fact that this code is completely unmaintainable. How do you know the difference between member1, member2, member3? Maybe member1 is a players health while member2 is a players magic power and member3 is yet another parameter a character has. When you start coding and you come back and look at this class a long time from now how will you know what member variable stands for what. The get/set methods allow you to specialize the name so that yourself or others you are coding with can easily use your code. There are a ton of other issues that I'm sure others will point out, but the main point is that this is not a good way to think about designing your classes. It will simply not work most if not all the time.

Share this post


Link to post
Share on other sites
Quote:
Original post by Sneftel
Quote:
Original post by cwyers
hi guys,
quick question about set and get functions(obviously) for a class with a lot of member values. Instead of making a set/get function for each member, just let the class user send the variable they want in the argument.

What would that gain you?


well, it keeps me from having to write a bunch of get/set functions lol

i was just thinking about reusing code and was like ... ugh i dont want to write
all these separate functions that programatically are identical...

just a though ... since this stuff is in development...it would probably suck even more to keep changing my switches around if i rename a member value]

ive never even heard of these associative containers...quick google search reveals...that im a noob.

thanks for the responses guys.

Share this post


Link to post
Share on other sites
Quote:
Original post by cwyers
well, it keeps me from having to write a bunch of get/set functions lol

Right, but now think: why are you writing all these get and set methods (member functions)? It's unfortunate that most formal educations emphasize the getter and setter as a means of encapsulation. The main reason that is given for writing getter and setter methods is that you can validate setter arguments and transform getter return values.

But here's the thing: methods should represent actions on objects (or messages passed to objects). That is, you should not care about the underlying data in order to perform an action (programming to an interface, not to an implementation). Here's an example: when writing a list class, do you write a set_tail( T * ) method, or do you write a push_back( T ) method?

That is not to say that getters and setters are always a bad idea. You will undoubtedly find a situation where they are suitable, but probably not too often. In any case, trying to write a 'centralized' getter or setter means that you should probably be adding structure to your data (the aforementioned map), or that you are writing it for the wrong reasons.


jfl.

Share this post


Link to post
Share on other sites
Quote:
Original post by cwyers

well, it keeps me from having to write a bunch of get/set functions lol

i was just thinking about reusing code and was like ... ugh i dont want to write
all these separate functions that programatically are identical...


That's the problem with design. Why are you writing all these functions? Why not just use raw variables?

You have no extra functionality attached to setters or getters, so just use POD type. A struct would be just fine.

From usability perspective, if you need to provide access to raw variable, you don't need to expose it at all - provide only functions that manipulate them.

Consider this:

class Entity
{
public:
void move_to( int x, int y, int z )
{
// check for collision
// check valid move
set_location_with_event( x, y, z );
}

void reset_position( void )
{
// check if we can reset
// reset the score and animation
set_location_with_event( default_x, default_y, default_z );
}

const Vector3D &location( void ) const
{
return m_location;
}
private:
void set_location_with_event( int x, int y, int z )
{
m_location.x = x;
m_location.y = y;
m_location.z = z;
events.locationChanged(this, m_location);
}
Vector3D m_location;
};



We can never simply put an object down somewhere, since it might mess up player location. So we need to check a lot of things.

In addition, users are now faced with meaningful methods that manipulate location, but hide the functionality from the users.

Even more, the location itself is manipulated in one method only, completely shielding all other functions from API changes.

And the location construct which is needed by rendering API is provided in a safe manner.

This is the whole point of OO - don't give user meaningless variables - give them functionality.

*Note: good functionality depends on understanding the problem. Above example could be completely wrong for some cases, and there are situations where allowing direct manipulation of location would be required or at least desirable.

Share this post


Link to post
Share on other sites
Quote:
Original post by cwyers
well, it keeps me from having to write a bunch of get/set functions lol
No, it wouldn't. It would just reformat them into one "uber-function" plus an enumeration. It wouldn't save you a significant amount of typing. It would just make things more confusing.
Quote:
i was just thinking about reusing code and was like ... ugh i dont want to write
all these separate functions that programatically are identical...

If you really don't think that you'll ever need any input validation, why not just make the variable public?
Quote:
just a though ... since this stuff is in development...it would probably suck even more to keep changing my switches around if i rename a member value
Or, worse, change a variable's type.
Quote:
ive never even heard of these associative containers...quick google search reveals...that im a noob.
Associative containers are not generally appropriate for this sort of thing, because the list of members is known at compile-time. Associative containers are for dynamically adding and removing properties at runtime.

Share this post


Link to post
Share on other sites
1) Do not over-encapsulate. In C# encapsulation is great, in C++ sometimes it is better just to make certain data members public. If you have to read/write to private data members and their are no specific rules that must be enforced, then there is a chance that those members should be public.
2) Learn the stl, the map and hash map (in the stdext namespace in MSVC 8) are good examples of associative containers.
3) Sometimes having "a lot" of member variables is (not always) a sign of poor OO design. Make sure that your object cannot be represented more simply. Often times you'll see that either a given data member can be calculated from others (ie: bitmapSize=width*height*bitsPerPixel), it does not need to be retained in your object (ie: a file name used only once to load data from file), or your use of objects needs to be reworked (do you find that many different objects do the same stuff? Are your implementations conforming to the quirks of your objects? Do you need to make something new every time you want to do something slightly different?).
4) In very rare occurrences it is actually beneficial to use a Memento pattern by grouping data members into a struct or all public object and making that the private member of your object.

5) Often times the more direct way is the correct way, do not add unnecessary indirection.

Share this post


Link to post
Share on other sites
Quote:
Original post by Antheus
Quote:
Original post by cwyers

well, it keeps me from having to write a bunch of get/set functions lol

i was just thinking about reusing code and was like ... ugh i dont want to write
all these separate functions that programatically are identical...


That's the problem with design. Why are you writing all these functions? Why not just use raw variables?

You have no extra functionality attached to setters or getters, so just use POD type. A struct would be just fine.

From usability perspective, if you need to provide access to raw variable, you don't need to expose it at all - provide only functions that manipulate them.

Consider this:
*** Source Snippet Removed ***

We can never simply put an object down somewhere, since it might mess up player location. So we need to check a lot of things.

In addition, users are now faced with meaningful methods that manipulate location, but hide the functionality from the users.

Even more, the location itself is manipulated in one method only, completely shielding all other functions from API changes.

And the location construct which is needed by rendering API is provided in a safe manner.

This is the whole point of OO - don't give user meaningless variables - give them functionality.

*Note: good functionality depends on understanding the problem. Above example could be completely wrong for some cases, and there are situations where allowing direct manipulation of location would be required or at least desirable.


I don't understand why you didn't just do this:

class Entity
{
public:
void move_to( Vector3D &v )
{
// check for collision
// check valid move
m_location = v;
}

void reset_position( void )
{
// check if we can reset
// reset the score and animation
m_location = Vector3D(); // assuming the default constructor resets it
// if not then you can do this m_location = defaultLocation;
}

inline const Vector3D &location( void ) const
{
return m_location;
}
private:
Vector3D m_location;
};

Share this post


Link to post
Share on other sites
Quote:
Original post by T1Oracle
I don't understand why you didn't just do this:
*** Source Snippet Removed ***


Read my last sentence - you don't know my requirements.

What if Vector3D is internal format, which isn't available to the user and is merely provided for the renderer.

Also, your example no longer fires the event.

And reset position doesn't use Vector's default position, but position defined by particular map.

I also don't need to put inline in front of methods since I use "any suitable" for inlining, and "optimize for speed".

This also demonstrates nicely why it's somewhat pointless to ask others advice on design, without providing exact and detailed use-case.

The enum solution might work - I don't know. It's just uncommon. A map solution might work as well. Or overriding subscript operator. Or auto-generating accessors from IDL. Or providing dynamic type system. Or component oriented object composition.

There are no absolutely "good" or "bad", "right" or "wrong" ways. There is just suitable and unsuitable design. Yes, even singletons have their rightful place in design. They are just frequently unsuitable.

Share this post


Link to post
Share on other sites
Quote:
Original post by Antheus
What if Vector3D is internal format, which isn't available to the user and is merely provided for the renderer.

Then make one that is (you could do a simpler one), it will most certainly be useful. 3D vectors do not need to be limited to the renderer, especially when you have a clear need for it elsewhere. Why not make things easier?
Quote:
Original post by Antheus
Also, your example no longer fires the event.

Trivial, you can add two more lines and fix that.
Quote:
Original post by Antheus
And reset position doesn't use Vector's default position, but position defined by particular map.

Why the vague terminology? Your example does nothing mysterious. Surely a vector isn't going to add more than 10 seconds of work to get your "particular map" to play nice with it. Meanwhile you get the benefit of only having to pass one variable, (a vector) and the utility of using the same pattern (which surely has use through out your code if you need coordinates in the first place) in other places.
Quote:
Original post by Antheus
This also demonstrates nicely why it's somewhat pointless to ask others advice on design, without providing exact and detailed use-case.

I agree although statistics are still a viable tool for reason. The less common is less common for a reason. Therefore, the reason for choosing the less common should be scrutinized more carefully.

Share this post


Link to post
Share on other sites
The only benefit that I can see is with regards to animation systems.

I have done a similar thing before, property names and types are mapped to integer property IDs, which can then be animated via keyframes or other methods. It is no more fun than writing out all the getters and setters. Especially when you can't use switch since the IDs aren't defined at compile time. I would do it differently if I were to redesign it.

Share this post


Link to post
Share on other sites
The problem with using many getters/setters OR the approach you propose is that you seperate the data from the algorithms, as the code that deals with the variables get put in a different class.

Objects are supposed to contain both algorithms and data. You either have to ask yourself "Why is this variable in here when the only stuff that touches it is over in that?", or "Why are the functions that touch this variable over in that?".

That said, there are certain Design patterns where this kind of thing makes sense. In that case you should know what the name of the design pattern is, and why you are using it.

Share this post


Link to post
Share on other sites
I think it could make sense if you had a massive library that your programs depended on which had dozens of configuration options whose state would need changed at unpredictable intervals.

For instance, with OpenGL you have things like glLightfv(GL_LIGHT5,GL_POSITION,l5pos); which may or may not be easier than glLights[5].position.set(l5pos); or something similar. I suppose it depends on the scope. I certainly wouldn't do it for something like entity health updates, although I could see enumeration for states eg. entity[23].setState(ES_SCARED_SILLY);

But then again, that's an entirely different situation.

If you have many, many variables that need updating, then it might be time for a reorganization. If you have something like:


class Squirrel {
float ambient_r;
float ambient_g;
float ambient_b;
float ambient_a;
float diffuse_r;
...
float position_x;
float position_y;
float position_z;
float velocity_x;
float velocity_y;
...
void setHungerLevel(float);
float getHungerLevel();
void setFavoriteFoodisNuts();
void setFavoriteFoodisBeans();
int getFavoriteFood();
...
};

then I would say it's time for reorganization. Can you be more specific in what kinds of variables you have such that you need to completely rearrange how you get and set them?

Note: I'm don't really think you've made anything as bad as that squirrel :)

Share this post


Link to post
Share on other sites
Original post by Antheus
I also don't need to put inline in front of methods since I use "any suitable" for inlining, and "optimize for speed".[\quote]
In practice, on most compilers, whole program optimizations aren't that great. I'd be explicit always.

As for the OP question:

That isn't a bad idea for writing interfaces that can be easily extended to support new data without changing the interface to the class. To support a new data type you simply need to supply a new ID (whether it be a enum or a hashed integer).

I also think associative containers like std::map and using a string for the key is very bad in practice. Doing string compares to find data isn't effecient and doesn't scale well with lots of data.

Though, this pattern is used frequently but not really in the context you are describing.

-= Dave



Share this post


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

  • Advertisement