getters & setters are bad, design patterns etc.

[TheIdeasMan]

I am going to have another bash at this - I am still in a small way sceptical about some aspects of this. There has been discussion about this before, so I am putting all my thoughts down to see what everyone thinks.

http://www.cplusplus.com/forum/beginner/101183/#msg544161

Continuing on from this messages & others in that topic, I would say this:

I think there is a difference between getting a raw attribute versus doing a transaction. For example - "What is your height?" versus "Here are your shiny shoes, please pay me $5". It is easy to see how getters & setters used in the context of a transaction could be very bad. From rude / threatening "How much money do you have?" to theft - "Take $1000". So I fully agree with the idea of using a design pattern for transactions as per the article ne555 quoted.

It is important to provide an interface for objects - without it they are pretty much useless. For a circle one should be able to enquire about it's centre point, Radius, Area & Perimeter. A get function for a raw attribute might be a necessary part of that interface, even when used in conjunction with a design pattern.

Also there is a distinction between a setter that is straight assignment (Bad) versus an Update type function which might do permission & validation checks & recalculate other member variables in the process. An Update function could also be part of the interface.

Then there is the level of knowledge of the programmer. Things like design patterns could be way too much for a beginner & far past the requirements for their assignment. Although one should strive to have elegant solutions, a lesser one might OK for small programs like a typical high school assignment.

So what about these points, presented in order from very bad to good:

1: Public variable

Good points: None.
Bad Points: Anyone can change it & invalidate the other member variables.

2: Protected variable

Good Points: Dead easy - it's not public any more & derived classes functions can access it directly.

Bad Points: It works really well, until the type needs to change or is extended:

Article wrote:

Today an "identity" is a name; tomorrow it's a name and ID number; the day after that it's a name, ID number, and picture.

Any code that uses it must be changed to reflect the new situation. Can't alter any computation because it's just a variable.

3: Private Variable with Public functions

Good Points: Can change some computation inside the function without the client being any the wiser. Any object outside the class can obtain the value of a raw attribute. Can provide functions that update several variables at a time, and those that display variable values. A selection of the functions form the interface for the class.

Bad Points: Still can't "Future Proof" against change in type or extension as above.

getters & setters for each member variable is just the same as making all the variables public. So the designer needs to decide what info should be available.

4: Promote The variable to a class

Good Points: Now we have a better chance to "Future Proof" the info. We can do all kinds of things in the class.

Bad Points: It is more complex.

5: Use Design Pattern/s

Good Points: There is now all kinds of flexibility, reuse, decoupling etc.

Bad Points: Even more complex, but worth it. There are some adverse consequences which need to be considered.


So it seems that trivial getters (returns something), and setters (straight assignment) are the real bad boys here. Compare to those functions that conditionally have the same result, but do checking & validation. Some functions might not need much checking - think getRadius.

It also seems that the classes Design Patterns deal with still have get functions in their interfaces, but they are coupled only to the DP class, and not to all the other classes.

There you go - that's my thoughts for today. I look forward to any replies, which I read tmrw because it's 03:00AM here - again!!

[MiiNiPaa]

Trivial getters are not bad if they are trivial not because you are designed it like that, but it turns out they are shouldn't be non-trivial in current implementation. If they are part of well-designed interface.
For example there is many ways to define ellipse. One of them is to define both focal points and major axis. Does that mean that function getFocalPoints() which returns a pair of points stored as private members are bad? No, because it makes sense for ellipse to have this method.
Even if we change implementation of ellipse to define it by storing both axis, center point and rotation angle it still makes sense to have getFocalPoints() method in Ellipse class, but it will not be trivial in this case.

[3qX21hU5]

I totally agree with your point of view about getters and setters.

Up until recently I was a firm believer that getters and setters are just plain bad class design and break the whole point of encapsulation. But then someone pointed out to me that not all getters and setters are bad class design.

The example he used is lets say you are creating a class and part of that classes functionality deals with times. The private members might look like this (Very simplified).

class Time
{
    int hour;
    int minute;
    int second;
}

Now if you just started to add getters and setter for every private member like int getHour() { return hour; } THAT would be amazingly bad class design, and would cause a huge amount of problems for users of the class if the class changed in the future.

But it isn't bad class design to declare a getter and setter that might look something like this.

class Time
{
    public:

        // Didn't include error checking or anything for simplicity
        string getTime()
        {
            stringstream convert;

            convert << hour << ":" << minute << ":" << second;
            return convert.str();
        }

        // Again no error handling or handling of different input
        void setTime(const std::string &time)
        {
            char throwAway;
            istringstream convert(time);

            convert >> hour >> throwAway >> minute >> throwAway >> second;
        }

    private:
        int hour;
        int minute;
        int second;
};

With this getter and setter all the user needs to know is how to input the time he wants. He doesn't know how that the time is stored in int and uses a stringstream to convert them. It also makes it much easier to change the implementation of the class without affecting the user that uses the class.

At least that is what he told me ;p but he has much more experience with C++ then I do so I'm willing to take his word on it.

But anyways just wanted to share my point of view on the subject, and plan on looking into good design of getters and setters more later. So thank you for the post IdeasMan.

[Script Coder]

My (very inexperienced) point of view is getters and controlled setters. Getters work the same as they would normally (i.e. they give you a value) (also note that this could be in what ever format, Zero's too). While setters should perform checks on the values to ensure they are valid, etc. Also, members that do not need to be changed should not be changed, or should only be allowed to be changed in specific ways. A trivial example would be an incrementer/counter class that would have a getter that gives the current count and a setter that only allows the user to add one. Any one care to explain what is wrong with my reasoning?

[MiiNiPaa]

It depends wht your Time class is defining.
TimePoint? In that case it is make sense to have hour() member function which will return hour in local timezone when time point takes place.
Does it defines duration? In that case you can have toHours() function which will return duration in hours.
If that class is defined as you shown that function will be trivial. But if you change implementation to store only seconds (Unix time) you should change your method and it will became non-trivial.

You should define interface before implementation and design it not from position "will it be trivial?" (You shouldn't think about implementation yet) but from position "will it help to support purpose of my class?"

[Disch]

The purpose of encapsulation is so that your class can represent a "thing", or an "object". This thing/object is (or at least should be) a singular entity with a clear purpose.

Getters/setters tend to dissolve that because you no longer are treating the object as a single entity, but instead are treating it as a collection of parts. Accessing individual parts of a singular entity violates encapsulation.

In those cases, where you have a collection of parts rather than a single entity, a struct (with public members) is often more suitable.


That said... these individual objects can have properties that describe them. And getting/setting them on some level may be necessary. MiiNiPaa's example of a getFocalPoints for an ellipse class is a pretty good one. Another good example is a situation like std::vector's size/resize functions, which could be thought of as getters/setters.

So really there is no hard-line rule that applies to every situation. It's all about context.

[Cubbi]

like std::vector's size/resize functions, which could be thought of as getters/setters

we call those "salient attributes", which are defined as "the documented set of observable named attributes of a type T that must respectively have/refer to the same value in order for two instances of T to have/refer to the same value"

Vector is a good example: size() and operator[] are salient attributes, but capacity() is not: two vectors with the same size and the same elements compare equal regardless of capacities.

The important point (also illustrated with the ellipse above) is that attributes are an interpretation, they are not the actual state of the object, as public data members or naive getters/setters would be. Most vector implementations don't have a size member, but there is "size" attribute in the vector's interface. The users see vectors as having a modifiable "size".

Such attributes are good (and, really, the only sensible) class design for value-semantic types (ellipse, time point, vector). But there are, of course, many other type categories.

[LB]

I agree with Disch, though I don't think MiiNiPaa's example nor vector size and resize are getters or setters.

It's the vector that manages its self (memory/allocation, capacity, etc), the code that uses the vector manages the contained elements.

[moorecm]

Simply put, I'm a firm believer that there should be no "C-style" structs in C++. It is never acceptable to lump data members in a struct and not explicitly define an API for it. (Even a poorly-designed API is better than no API.)

[LB]

@moorecm

template<typename T>
struct Vect2D
{
    T x, y;
};

[moorecm]

Consider a project at large scale. When you find some bug where the coordinates are bad, it's typically not discovered where the problem is created--you just observe the symptom after the fact.

How do you find the bug?

...

There are plenty of examples of the benefits of NOT leaving the members public; I'm not going to try to convince you.

[LB]

moorecm wrote:

How do you find the bug?

How do you find the bug if coordinates are sttd::strings? std::string has no public or protected data members, everything is with member functions. So by your logic the bug should be easy to find, right?

[MiiNiPaa]

there should be no "C-style" structs

std::pair

When you find some bug where the coordinates are bad

Who said about coordinates? Vector is a mathematical primitive. It shouldn't know about state of self and it makes little sense for it to manage itself.

Helper classes often is a plain C-struct (like node containing list element)

It is fine to use C-structs if they are just an aggregate of values. In that case public values IS the class interface (C++ Coding standards, item 41: http://www.gotw.ca/publications/c++cs.htm )

Value aggregates (also called "C-style structs") simply keep a bunch of data together but do not actually add significant behavior or attempt to model an abstraction and enforce invariants; they are not meant to be abstractions. Their data members should all be public because the data members are the interface.For example, std::pair<T,U> is used by the standard containers to aggregate two otherwise unrelated elements oftype T and U, and pair itself doesn't add behavior or invariants.

And there is use of different language modules in your project. Many of them C-compatible, so you should use POD types to send data to them

[Disch]

mooercm wrote:

I'm a firm believer that there should be no "C-style" structs in C++. It is never acceptable to lump data members in a struct and not explicitly define an API for it.

I very recently wrote a quickie program that had some application-wide user preferences that were applied throughout the program. It basically acted as a bunch of "configurable constants", most of which were for display preferences... like which colors represent which things, how far apart things are placed, etc.

The easiest and simplest way I could think of to implement this was with something along the lines of a singleton struct with public members. Two "no-no"'s that many people (including myself) flip out about when they see it being (ab)used.

The struct had about a half dozen colors and maybe 3 or 4 ints. Each had no relationship to each other aside from the fact that they were user configurable.



If this was not to be handled as a struct with public members.... how would you have handled it? The only other way I can think of that would be feasible would be individual, naive getters/setters for all members which would have been a complete waste of time, IMO. Unless you have some other alternative?

[moorecm]

It's easy to become lazy when designing a class (or lack there of). The issues presented here are not unfamiliar to me. "Outrage" is the usual response from my point of view. I get that. What's bothersome is the lack to trying to understand my perspective.

I've written a lot of software, not that I'm claiming any of you haven't, and I have never once came across a struct that wouldn't be better suited as a class. In fact, many times I have converted structs to classes for various reasons.

API Design for C++ by Reddy lists about a dozen reasons to encapsulate members.

L B wrote:

How do you find the bug if coordinates are sttd::strings? std::string has no public or protected data members, everything is with member functions. So by your logic the bug should be easy to find, right?

Easier, yes. Debugging is one of the benefits of encapsulation as described in the book cited above.

MiiNiPaa wrote:

std::pair

A pair is not without member functions. For example, have you ever tried comparing 2 of them with operator< ?

[cire]

A pair is not without member functions. For example, have you ever tried comparing 2 of them with operator< ?

Does an int, then, have member functions? For example, have you ever tried comparing two of them with operator< ?

I understand where you're coming from. There is always a benefit to using getters/setters to increase encapsulation over using direct access, however some folks think the cost of doing so outweighs the benefit, and in many cases they may be correct when writing that "quickie" program. Not every code snippet is meant for reuse or extensibility or even to have a high level of maintainability.

[Lachlan Easton]

So how WOULD you handle Disch's example?

[Disch]

It's easy to become lazy when designing a class (or lack there of).

I don't think it's a matter of being lazy. I think it's a matter of not wasting your time and energy. Needlessly putting things behind getters/setters that don't do anything provides literally zero benefit.

All it does is require more code, which means more time invested and more possibility for bugs.

What's the value of wrapping x,y,z members of a "Vector" or "Point" class behind a get/set interface? I honestly cannot see one.


Also, I still am very interested in how you would approach the scenario I mentioned in my previous post. I really hope you respond to it. I love to soak up different people's approaches and ideas.

[LB]

@Disch in ChessPlusPlus we are using instances of classes that extend the Configuration class based on the thing using them (e.g. BoardConfig, GraphicsConfig) and different instances correspond to different loaded config files (currently only one config file, but mods might need more).

[tition]

I find the getter/setter paradigm not well enough designed. Most of the time, I need my members to be modification-protected, but not read protected. For example:

class MyItem{
public:
double readOnlyPrice;
bool SetPrice(double inputPrice)
{ if(inputPrice<0)
  { //do some error handling here...
    return false;
  }
  this->readOnlyPrice=inputPrice;
  return true;
}
};

So long as you don't modify readOnlyPrice, there is absolutely no point in having a GetPrice() method - code that doesn't do anything shouldn't be present. However, you do need a SetPrice() method.

Note that the above code will work quite well in practice. True, readOnlyPrice is public, but, because of its name, every sane programmer will think twice before modifying it, and if not, well, you will know who to fire first.