getters & setters are bad, design patterns etc.

Pages: 1234... 7
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
struct MyItem
{
    using Price_t = double;
private:
    Price_t price;
public:
    Price_t const &Price; //read-only, updates automatically

    MyItem() : Price(price)
    {
    }

    bool SetPrice(Price_t newprice)
    {
        //...
    }
};


Pros: can easily be swapped out for a mimic
Cons: swapping out for a mimic may cause some poorly-written code to have errors
Last edited on
Disch wrote:
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.

The value is encapsulation. If at any point in time you want to change your Point class to have a different internal representation (say, from cartesian to polar coordinates or from absolute to relative coordinates), one doesn't (necessarily) need to modify the interface. This is true for any type. Just because one can't see the need for changing it now, doesn't mean the need won't arise in the future.

Of course, the value does come with a cost and obviously opinions and experience vary as to whether it's worth the trouble. In practice, I tend to think it's not worth the trouble -- but, it's worth noting my practice/experience is not professional and mostly on projects very few people will see.


tition wrote:
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.

That's incredibly obtuse. "We don't need to use language features that enforce access restrictions, because we name our variables readOnlyXXX, and that way we can save a whole line of code!" Surely the sane programmers have already left.

Ironically, accessing the variable without access restrictions, would require more typing due to the regrettably long name.
L B wrote:
@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).


Thanks for this. That's definitely a good use case for inheritance and modularity (not having a singleton). I can definitely see the value in that.

But when it comes down to individual config options within the GraphicsConfig class.... do you write getters/setters for all of them?

In this situation I really don't see a "configuration" being a single entity apart from how it's stored. Does any one config option depend on the status of another? Are any of them interconnected in any way?

cire wrote:
If at any point in time you want to change your Point class to have a different internal representation (say, from cartesian to polar coordinates or from absolute to relative coordinates)


Vectors, by definition, are always relative.

And changing internal representation from cartesian to polar coordinates is a big enough change that it would demand the interface be changed or else performance would tank. Doing trig calls for every setX() function... then again for every setY() function would be completely unacceptable in nearly all practical applications. (yes you could get around that postponing the trig calls until they're absolutely necessary, but that is a whole other can of worms I won't get into).

Furthermore, many common mathematical functions that vectors are used for rely on it being in cartesian coordinates (*cough*dot product*cough*).

And lastly, even if you had a need to work in polar coordinates after a long-standing cartesian system... a better solution would be to create an entirely different PolarVector struct, and have (explicit) ctors to convert between the two.


cire wrote:
one doesn't (necessarily) need to modify the interface. This is true for any type.


I would disagree. Some types are so fundamental that their direct access is their interface. Granted this is not common by any meaning of the word... but it certainly exists.

I wouldn't say my 'config options' example would qualify... but a Vector certainly would.

Just because one can't see the need for changing it now, doesn't mean the need won't arise in the future.


There is truth to this... and I'm certainly in favor of writing code that is adaptable and expandable. However there are limits. At some point you have to say "This is what this class does".

It is literally impossible to plan ahead for every conceivable change down the line. To even attempt to do so invites increasingly obfuscated code and stifles productivity.

Case in point, in an little indie group project I'm working on... one of the teammates was writing a class to manage a relatively simple task. We gave him the requirements of what we needed the class for and how it was going to be used. ~4 days later he comes back with a huge hierarchy of templates. Maybe 6 or 7 classes, well over 1000 lines, and code so dense nobody else wanted to deal with it.

When I looked through the code and started asking him Qs about it... like "Why did you do X?", the answer was typically "it would be useful if we need to do Y with the class". ... But we didn't need to do Y with the class.

Anyway long story short... we scrapped the stuff he wrote and someone else on the team spent half a day (vs 4 days) to write a ~150 line class that did what we needed.


TL;DR

The moral here is... be flexible... but also be reasonable. Don't let an endless stream of "what ifs" force to you write obfuscated code.
I'm going to skip the comments and reply straight to the OP.

Getters and Setters really should be used across the board when you need to get or modify a class member.

Why?
When you code the member it maybe a simple member (e.g bankAccount.balance) it may not need any validations. But you cannot guarantee this is going to be the case in the future. Adding the get/setters later is going to be a lot more costly than just doing it straight away and having the structure in place to add validation in the future.

It's a hell of a lot easier to modify an existing getter/setter than it is to find every instance of a variable and modify it to use a newly created one. And to the client/company this time costs money; and it could have been a cost mostly prevented by just using the getter/setter in the beginning.

FWIW my setter/getter would look like: (yes inline until validation is required)
1
2
double balance() const { return balance_; }
void set_balance(double new_value) { balance_ = new_value; }

Last edited on
I think for POD types (such as mathematical vector, colour, various utility structs) public member access is acceptable. To go to an extreme, you wouldn't want to write someInt.GetValue().
Last edited on
Zaita wrote:
Getters and Setters really should be used across the board when you need to get or modify a class member.


The whole point of encapsulation is that outside code shouldn't even know what the members are. If you are writing getters/setters for stuff, it's a sign your classes are poorly formed.

IMO anyway.
I think for POD types (such as mathematical vector, colour, various utility structs) public member access is acceptable. To go to an extreme, you wouldn't want to write someInt.GetValue().


A object holding just data is typically defined as a struct with public members and no methods.

The whole point of encapsulation is that outside code shouldn't even know what the members are. If you are writing getters/setters for stuff, it's a sign your classes are poorly formed.

IMO anyway.


I think for basic software and the standard user->data->database business applications this is definitely going to be mostly true. (applications that map to business processes etc)

But when you step away from that type of development in into scientific, 3D, real-time processing/rendering, gaming then you'll find that your guide is the exception instead of the rule. These kinds of applications tend to be a lot more complex and have components that heavily rely on the parameters of other components for functionality.

Edit: It's also worth noting that sometimes you have to do away with encapsulation for speed benefits when working on highly optimised software as well.
Last edited on
A[n] object holding just data is typically defined as a struct with public members and no methods.
Typically yes, but there are still instances where these structs will have member functions.
In reality a struct and a class are almost identical in C++. The only difference is that a struct defaults to public and a class doesn't. The only difference is in how you use them in your code.

This will come down to your coding standard. The standards I use do not really allow methods on structs. If only for a solid way to differentiate them from classes.
Whether you call them structs or classes is irrelevant; having public data members can make for good code in situations.
There is little difference between struct and class.
There is a huge difference between a struct and a class.
Last edited on
> Getters and Setters really should be used across the board when you need to get or modify a class member.
Make the getters and setters private. ¿have anyone listed a good reason to make them public?
If they are public, then you are making the client code responsible for managing your class.

@OP: Protected means public for derived classes. So you break the encapsulation of the base class.

> Furthermore, many common mathematical functions that vectors are used for rely on it being in cartesian coordinates (*cough*dot product*cough*)
You should stop smoking
By definition `uv = |u| |v| cos \theta', wouldn't be a problem in 2D
(however have no idea how to expand it for more dimensions, and it's inefficient in 2D)


About the `time' example, ¿why do you need a toHour() operation?
If 'time' class actually defines a 'duration' there is some sense to be able to convert into some measureable units. We can have toHours(), toMinutes() and so on member functions if there is a need for them in program class was designed for.
Or we can design a multipurpose duration class like in chrono library.

however have no idea how to expand it for more dimensions
It is still product of their absolute values and cosine of angle between them.
I mean how to compute the angle between them
It boils down to finding angle between two intersecting lines, which is trivial.
Wow - 1 day & 2 pages of replies ! Thanks everyone.

It seems that trivial and / or naive get/set functions are bad news, but I can't help thinking there are quite few situations where sensible use is valid.

Disch wrote:
The whole point of encapsulation is that outside code shouldn't even know what the members are. If you are writing getters/setters for stuff, it's a sign your classes are poorly formed. IMO anyway.


This is the nub of what I don't quite get. Thinking about basic Point, Rectangle, or Circle classes - how are these going to be useful if one doesn't have access to the value of their data members?

Can get functions not form a valid part of the object's interface? And any kind of update or modify functions (a set function in disguise) with validation code, could be an equally valid part of the interface?

How would one implement the DrawTangentLine problem, given 2 circles?

If I used a Command Design Pattern, I still need get functions in the Circle (read any drawing object) interface, it's just that they are coupled with the Command class as opposed to all the classes in the application.

A much more complicated example, which I am working on at the moment:

http://www.icsm.gov.au/gda/gdatm/gdav2.3.pdf
Vicenty's Inverse Formulae
Chapter 4, page 15 - page 20 in pdf


(hopefully one has LOTS of spare time if one wants to read & absorb the whole thing! )

I am just doing this one part for now, but would like to implement code for most of the formulae in the whole document. All the stuff is related (the whole thing is essentially about calculations for 2 points) - so potentially it could all be in one class. But there is already lots of code - Vicenty Inverse involves 14 formulae, with 40 variables to work out the answers which are only 3 variables. I don't particularly want a class with more than 1500 LOC, so I will probably split it into several classes - now info needs to be passed between the classes.

Also, what happens in a GUI Form which contains info from several classes - how does one populate a text box with the answer from some calculations in a class?


void set_balance(double new_value) { balance_ = new_value; }

That's one completely inappropriate function for a bank account to have.
closed account (3qX21hU5)
1
2
3
4
5
brandon.set_balance(999999999.23);

cout << "Mwhaha I'm rich!!!" << endl;

brandon.leave_country_to_go_to_tropical_island();
Last edited on
Last edited on
TheIdeasMan wrote:
This is the nub of what I don't quite get. Thinking about basic Point, Rectangle, or Circle classes - how are these going to be useful if one doesn't have access to the value of their data members?


You're right. Which is why Point, Rectangle, and Circle classes are typically not encapsulated... or are typically not even classes (but are instead structs).

The only places I've really seem them represented as encapsulated classes is in trivial and unrealistic examples to illustrate how polymorphism works. Or if they're representing something outside of just the basic geometric shape.

Unfortunately I don't have time to respond to the rest, as I have to get ready for business. I'll try to reply this evening if I remember and have time.
Pages: 1234... 7