Many simple get/set members

[Carradin]

My class has about 6 members which may go up to 12. They are quite simple types which don't really need any funny business and can be set directly. I've always heard that public members are 'evil' and the class interface should consist of public methods. I really don't see the point of making get/set methods as they are just doing setting and getting the members. I was thinking of putting them in a struct and having a getter and a setter method, but I thought it might be annoying when you just want to set one variable of the struct. It would look like this:

Struct myStruct = myClass.getStruct();
myStruct.var = 5;
myClass.setStruct(myStruct);

as with a setter it would just be:

myClass.setVar(5);

My class does have a lot of methods which do not set or get, so I do not think my design is wrong. My class may need to be notified when a member changes so maybe public members aren't the best choice. I would be grateful if you could get me out of this dilemma and show me the right way :).

[kbw]

I don't think it's worth jumping thru hoops just to save a bit of typing for get/set methods.

[Carradin]

but do you think its acceptable to have 12 get/set methods which basically well set and get the members?

[kbw]

Yes.

[webJose]

Agreed. Best practice is to provide get/set accessors.

If you find yourself typing too much, maybe a simple macro helps for the simplest ones:

//Assumes private variable is called m_propName.
#define GET_ACCESSOR(propName, propType) propType get##propName() const { return m_##propName; }
#define SET_ACCESSOR(propName, propType) void set##propName(const propType &newVal) { m_##propName = newVal; } 

NOTE: Untested, but I think it works or at least gives you an idea of what you can do.

[Peter87]

If your class just contains a bunch of data members without any relation to each other I don't think it's wrong to use public and skip getters and setters.

[TheDestroyer]

public members are evil only if you're using complicated types in your class that need careful managing, in my opinion. For example, if you have a vector in your class, the user should not have access to it, because this vector maybe be linked to some other functions and could get your class to fail if the user, for example, calls the method clear() deliberately.

And remember, having get/set methods helps with version consistency. If you're creating a huge project, the user shouldn't see what the hell goes on inside your monstrous classes. He should be able to use them easily in every version without needing to change his code. public access to member variables could conflict with that concept.

But honestly, for small home-made classes... who cares?!?!?

[ne555]

getters and setters are evil http://www.javaworld.com/javaworld/jw-09-2003/jw-0905-toolbox.html

I don't see a big difference between public members and setters/getters. You'll end with no encapsulation and with the logic in the client, instead of the class.

My class may need to be notified when a member changes

then (probably) the logic is in the wrong place.
Your objects should handle their state. The user may tell them to perform actions, and as a consequence the state may change. But if you let him touch directly, you may end invalidated.
A little more information about your classes will be appreciated

@Peter87: ¿could you provide an example?
@TheDestroyer: It does not matter how complex an object is. Is as easy to break an invariant just touching an int (by instance

{vector<int> v; 
v.size_ = 42;}

the user shouldn't see what the hell goes on inside your monstrous classes.

the problem is that the accessors will inform him.
He shouldn't need to use them.

But honestly, for small home-made classes... who cares?!?!?

The learning process. If you are doing a hello_world incorrectly...

[clanmjc]

You can also use a combination of get/set and struct if your data has value semantics.

myClass.GetStruct().xval = 1;
myClass.GetStruct().yval = 200;
//etc...

Or

someStruct &structRef = myClass.GetStruct();
//Manipulating structRef manipulates "myClass" struct directly
structRef.xval = 1;
structRef.yval = 30;

Or

someStruct values;
values.xval = 3;
values.yval = 5;
values.zero = -500;
myClass.SetStruct(values);

All these different implementations have their own advantages and disadvantages.

My class does have a lot of methods which do not set or get, so I do not think my design is wrong. My class may need to be notified when a member changes so maybe public members aren't the best choice. I would be grateful if you could get me out of this dilemma and show me the right way :).

Sounds like you have a lot of things going on with this class, without seeing your class I cant really give you any suggestions there.

[Carradin]

Sorry for the really late reply!! Here's my class

struct Node
{
    double x;
    double y;
    double dir;
    double corner;
    double state;
}

class Curve
{
  public:
      Curve();

      void addNode(idouble x, double y);
      void insertNode(int index, double x, double y);
      void deleteNode(int index);

      void moveNode(int index, double x, double y);
      void setCorner(int index, double corner);
      void setIncDir(int index, double dir);
      void setOutDir(int index, double dir);
      void setState(int index, int state);
      void setIncState(int index, int state);
      void setOutState(int index, int state);

      void setClosed(int index, bool closed);

      //Transformations
      void translate(double x, double y);
      void rotate(double angle, double pivotX, double pivotY);
      void enlarge(double scaleFactor, double pivotX, double pivotY);
      
      //Optimization
      void S_splineFeasible();
      void S_splineFeasibleSmart();
      void optimize(bool flag_coarse, bool full_refresh);
      int S_Curve(int index, double *rx, double *ry);
      
      int save(fstream &file);
      int load(fstream &file);
      
      //Getters
      int getCount() {return count;};
      bool isClosed() {return closed;};
      double* getNode(int index) {return nodes[index];};
      
  private:
      void pointChanged(int index);

      Node* nodes;
      int count, coarseRes;
      bool closed;
      bool *flag_feasible, *flag_optimal;
};

Theres probably a lot of sloppy stuff here, the setting is done by the methods, and the reading is done by getting the struct Node. I agree, it doesnt sound right :P. Basically the user is specifying the Node struct and then the Optimization and Transformation methods change it around. The Curve can be plotted with S_Curve.

In the struct Node, the only really funny one is state, which cannot be set by just setting the variable. the SetIncDir method isnt so important as it just changes the node's corner with some simple calculations. Whenever a change is made to a Node, I need (its not maybe anymore) to run the pointChanged method as it does some stuff.

I would like to learn the proper way of doing things and not just some work around. Thanks in advance!

[kbw]

As your persisting to and from streams, you could use the base istream/ostream. This would allow you to persist on any stream rather just files with no extra code

      int save(fstream &file);
      int load(fstream &file);

The change would be:

      int save(ostream &file);
      int load(istream &file);

The struct Node this is ok. But as it's only used inside Curve, you could make it a local struct declared in the private section. That way it wouldn't be confused with any other kind of node that you may add later.

[Carradin]

Thanks for the tips, I will go through with that, but I'm still unsure about class design.

@ne555:
I've read the article you suggested and some others, which makes my class design seem wrong becuase of too much access to the data.

the problem is that the accessors will inform him.
He shouldn't need to use them.

Could you suggest how I could improve my class design and reduce the accessibility. I can't really see how, because Curve is supposed to be have his data set, he's not going to choose some random Point to be his x, y. However the optimization does modify the direction.

[clanmjc]

void addNode(idouble x, double y);
void insertNode(int index, double x, double y);
void deleteNode(int index);
void moveNode(int index, double x, double y);
void setCorner(int index, double corner);
void setIncDir(int index, double dir);
void setOutDir(int index, double dir);
void setState(int index, int state);
void setIncState(int index, int state);
void setOutState(int index, int state);

Are all these used to "set" the node(s) that have just been added/inserted? If so, this interface can be drastically reduced by inserting and adding actual Node objects instead of just the x/y. This may not remove all of these methods but if what I gather is correct a good amount of them.

void addNode(const Node &newNode);
//Where newNode has x/y, corner, direction or whatever else you need already set. 

Do these methods really only get used once (when setting for the first time) or are they constantly called on existing nodes (or again only on new nodes)?

To add to what kbw has stated, if you do change it such that the Node is exposed (to get rid of the setters) you can always put the Node struct in a namespace.

namespace Curve
{
struct Node
{
    double x;
    double y;
    double dir;
    double corner;
    double state;
}
};

In the struct Node, the only really funny one is state, which cannot be set by just setting the variable.

I'm confused here, who is the manager of a Node's state, the Node itself or the Curve object?

Just taking a guess by looking at the header, really Curve's interface looks like it could be limited to these (including the Add/Insert/Remove methods as well, which I didn't post)

//Transformations
      void translate(double x, double y);
      void rotate(double angle, double pivotX, double pivotY);
      void enlarge(double scaleFactor, double pivotX, double pivotY);
      
      //Optimization
      void S_splineFeasible();
      void S_splineFeasibleSmart();
      void optimize(bool flag_coarse, bool full_refresh);
      int S_Curve(int index, double *rx, double *ry);
      

      int save(fstream &file); //Technically if it were me, I would not have these as members but non member
      int load(fstream &file); //Same for this, and I would make them friends (sometimes friends can be used to keep things encapsulated... whilst breaking encapsulation.
      
      //Getters
      int getCount() {return count;};
      bool isClosed() {return closed;};
      double* getNode(int index) {return nodes[index];};

[Carradin]

Do these methods really only get used once (when setting for the first time) or are they constantly called on existing nodes (or again only on new nodes)?

The methods are used on existing nodes and not straight after you add a node (well you can i guess). So the node will be changed frequently.

I'm confused here, who is the manager of a Node's state, the Node itself or the Curve object?

Well, there are three methods that modify the state, setState just is basically setting the variable state, while setIncState and setOutState do some funny business with state. So I guess to answer your question the Curve manages the Node's state, however none of the Curve members are needed to manage the Node's state.

Could you please clarify on why you'd prefer the save and load methods to be outside the class?

[Carradin]

I would think the best method is to have getNode and setNode and change Node into a class if i need any managing. However I thought it might be long and inefficient just to modify one field, as you have to get the Node, change it, then set it again.

[clanmjc]

The methods are used on existing nodes and not straight after you add a node (well you can i guess). So the node will be changed frequently.

Are the methods used through other methods of the Curve object. e.g. translate calls setCorner/moveNode etc. Or are users of the Curve object calling the methods directly?

Well, there are three methods that modify the state, setState just is basically setting the variable state, while setIncState and setOutState do some funny business with state. So I guess to answer your question the Curve manages the Node's state, however none of the Curve members are needed to manage the Node's state.

Are these required to be public or they are used internally in the Curve object? Curious, what is it that the state does? Is the state dependent on any of the members of the Node?

Could you please clarify on why you'd prefer the save and load methods to be outside the class?

Basically for decoupling and reusability. Maybe your Curve class can be reused somewhere else where there is no file I/O, or maybe later on you move to a Database for the I/O or both DB and File. By adding this interface publicly, anyone that uses Curve has access to it whether they need it or not. It just keeps things clean and allows flexibility.

I usually have I/O manager classes that do this separate from the actual data classes (which is what I would call Curve, a data class).

There appears to be a lot of exposure to the Curves internals, I'm just trying to talk through it with you so that I can better make suggestions. For instance, every one of those methods also has an index parameter, which tells me that the user of the object also has to know the index for the correct node within the Curve object that it wants to manipulate. This is why I am asking things like, are the methods actually used outside of the Curve object directly (maybe some are but maybe not all) or are they used internally for some of the other public methods as in my example above.

Lets say that you could set your node up before you add it, and add it as I have suggested. Do you still call the setters? Or do you call translate/rotate/enlarge publicly, and use the parameters passed to them to then call the other setters? See where I am going?

[Carradin]

Are the methods used through other methods of the Curve object. e.g. translate calls setCorner/moveNode etc. Or are users of the Curve object calling the methods directly?

The users of Curve call these methods, they shouldn't be used within the class, as the struct Node can be set directly.

Are these required to be public or they are used internally in the Curve object? Curious, what is it that the state does? Is the state dependent on any of the members of the Node?

The state is only required for the Curve's optimization functions, its not dependent on any other members of Node. All members of Node are independent. The user may want to see the state or any of the fields for displaying reasons.

[Carradin]

Ok, maybe I should explain further, this class is in a GUI project (using Qt) where there is a Graph class above it which stores an array of these Curves. Now, the transformations change all nodes in the Curve and are rarely used, but the five fields of Node are changed often via the graph or a widget which allows the user to type in values. There is a selected Node integer which saves the index of the currently selected Node.

Now that I think of it, the Curve class doesnt manage itself, its like a database for the graph. When theres a mouse click on the graph I check the distance of it to all the nodes to find out which Node was pressed, and change the selected Node accordingly.

But there are some reasonings to why I did that, I wanted the Curve class to not be Qt specific. If someone just wants the points of the Curve then there is no GUI stuff there, so he would use a purely C++ class (as I call it ). But now I'm catching on to what your saying, there really shouldnt be any need to change the Node after you add it. It would be like:

curve.addNode(Node(100, 100));
curve.addNode(Node(200, 200, 90); //Maybe I'll add some Node constructors
curve.optimize(true, true);
double, x[1000], y[1000]; //I just realised I could probably use std:lists
curve.S_Curve(x, y); //I'll make this get points of the whole curve not per piece
//Do stuff with the points 

However, my actual project is in a GUI app so the Nodes need to be changed often. I would still like to keep this class and also have the GUI version of it which would require the Nodes to be changed. Do you have any ideas on how to do that? Maybe I could inherit this one. Thanks a lot for your help, I think I'm starting to understand things now :P.

[clanmjc]

You could create an interface (abstract class) using inheritance all this interface does is support the GUI version that you need. By using an interface you keep the pure c++ object as simple and clean as possible and reusable.

Also, just because you have a lot of getters and setters doesn't mean you've done anything wrong or incorrectly, sometimes this is going to be the case and then you come to this same crossroad you are in. Do I use a struct and access the public members of the struct directly, do I want public accessors/mutators, it really depends on the implementation you choose. You can argue any implementation into the ground as being the right way or wrong way. Have fun!

Also to this point

I would think the best method is to have getNode and setNode and change Node into a class if i need any managing. However I thought it might be long and inefficient just to modify one field, as you have to get the Node, change it, then set it again.

If you return a reference to a node that needs to be changed then there is no real over head (other than calling the method itself, which is the same as calling methods of individual parts of the nodes, but more efficient than calling the parts individually), then you change the members you need to change and you are done.

[Carradin]

Okay, I think ill make the Node a nested class within Curve, as managing is needed. Thanks clanmjc and ofc everyone who contributed for your time, this discussion helped me understand whats going on a lot better :P.