using enums to generalize set and get functions

Started by
14 comments, last by David Neubelt 16 years, 10 months ago
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.
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.
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 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.
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.
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.
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.
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.
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.
Programming since 1995.
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;};
Programming since 1995.

This topic is closed to new replies.

Advertisement