Refactoring a class full of getters and setters

[Kevin211]

Hey everyone, I haven't written any C++ code in a long time, but I recently started a new project and I'm using SFML. Years ago when I was in highschool, I actually wrote several projects in SFML, and I figured I would reuse some of my code. But this question isn't about SFML, it's about software design.

The code that I'm about to show you is just a header file, because I think that's all that's needed for this discussion, but what immediately jumped out at me when I looked at this header file is that the code seems over-engineered and like it has a lot of redundancies, but it also does look clean and nice, and it's easy to read.

I was hoping that someone here might have a bit more experience than me with software design, and maybe I could get some ideas for how I should refactor it (or perhaps this is good, and I shouldn't refactor it). Anyways, here goes:

class TextButton{
private:
    sf::Text button;
    sf::FloatRect collision;
    sf::Color defaultColor;
    sf::Color hoverColor;
    bool mouseClicked;
public:
    TextButton(const sf::Vector2f& position,
               const sf::Color& defaultClr,
               const sf::Color& hoverClr,
               const std::string& text,
               const int& fontSize);

    bool pollClicked();
    bool isHovering();
    void draw();

    //setters:
    void setPosition(const float& x, const float& y);
    void setHoverColor(const sf::Color& color);
    void setPosition(const sf::Vector2f& pos);
    void setColor(const sf::Color& color);
    void setText(const std::string& text);
    void setFont(const sf::Font& font);
    void setWasClicked(bool c);

    //getters:
    sf::FloatRect getCollision();
    sf::Color getDefaultColor();
    sf::Vector2f getPosition();
    sf::Color getHoverColor();
    const sf::Font* getFont();
    std::string getText();
    bool getWasClicked();
};

Alright, so basically, the way this works is that the sf::Text object at the very top of the class is an object from SFML, and it's the actual text that's being drawn to the screen. All of that is handled by SFML. I use this object to emulate a text button, and basically the functions within the class will check to see where the mouse is in relation to the sf::Text object in the render window, and if it's hovering over the sf::Text object, the color will change to the "hover color" and if clicked, then pollClicked() returns true. That's all fine and dandy, and exactly what I need it to do. The three functions below the constructor handle all of that functionality.

But the problem is all of the setters and getters. I think that I could actually get rid of almost all of those, and just make the sf::Text object public, because most of those setters and getters are just one-liners in the actual function's source code, and it literally just calls the corresponding functions for the sf::Text object. It is possible that if I did that, you could use this class in a way that isn't intended, such as changing the color of the sf::Text object to something other than default or hover, but it would just get changed back whenever something triggers it to change to the default/hover color, and it shouldn't break anything; therefore I don't think it would be such a bad idea.

I want some opinions on this class though, like let's say this was part of some big project that I'm working on with other people, like for a job or something, which design should i go with for this?

[AbstractionAnon]

My getter/setter rant:
I'm not a fan of getters and setters when used to essentially make every private variable public. If you're going to do that, you might as well make the variables public and eliminate the getters and setters entirely.

Don't get me wrong, I'm a big believer in private variables. Private variables should be manipulated by functions that implement specific ACTIONs that are relevant to your class.
Getting or setting a variable is rarely an action that is relevant to what your class is representing.

If you find yourself blindly writing getters and setters, then you need to step back and think about what actions are performed on your class and how you're going to implement those actions.

p.s. I know it's popular these days to teach writing getters and setters, but from my point of view, blindly writing them to make private variables public is a concept that should be quickly unlearned.

[seeplus]

Much of class teaching is to always have member variables private with getters/setters. The reasoning is that the underlying storage of the data is a matter for the class and that the class user shouldn't/doesn't need to know how the data is stored. If consequently the manner of storage is changed, then the class setter/getter is changed but no changes to the class user are needed. If you don't use getters/setters then if the way the data is stored then all the class user code will probably have to change as well. Also getters can perform data validation to ensure that at all times the data held by the class is valid.

[dhayden]

AbstractionAnon and seeplus both make good points. Also consider some downsides of getters/setters: you can't take the address of the data or get a reference to it. You can't increment/decrement it with ++ or --.

So how do you decide if your class should use getters/setters or not? Consider the context. How many programs will use the class? How hard is it to change them if you modify the underlying storage? Do your getters/setters just get/set variables without any other side effects? On one extreme is a self-contained program like a homework assignment where getters/setters just get/set private data with no other side effects. In that situation, you get all the costs and none of the benefits. On the other extreme is a widely used library where changing the API would require hundreds or thousands of programs to be modified. In that case, getters and setters might be essential.

In you specific case, it looks like there's good reason for the getters and setters because they obviously don't just set/get the data. For example, there's a setFont() method but no font member.

[jonnin]

slightly on topic, I hope...
less code is better code in general. Don't overthink this -- pick the algorithm before you try to trim code (that is, don't use bubble sort just because its smaller). And there is a happy medium: everyone here can write unreadable cramped code to show off, so making it smaller has to make it equally or more readable to the average intern.

Why is less code better?

-Everything you write is a chance to introduce a bug. The less you write, the fewer bugs, and considering things like functions for repeated blocks, if you DO have a bug, you can fix it in one place, one time.

- the more bloated it is, the harder it is to read. I am reading it.. WHY did this guy put in this variable, it must be important ... no, it looks like its just a copy of another variable for no good reason... grumble..

- big chunks of code make it difficult to figure out correctness. the comments say its a bubble sort, but there are 42 lines, 10 between the inner and outer loop, what gives?!

That circles right on back to getters and setters. Echoing above, I only use them if they DO something besides assign a value or if the tool is going to be wrapped in a library and distributed where having full access allows them to do something extra stupid. Most of the time, that boils down to 'these setters validate before setting'. I don't like to do THAT either, because I like to share my validation across everything, not have one per value per object, but on occasion you have something radically different. And that creates a problem: if you have one idiotic value that MUST be validated by the class itself and needs a setter, at that point this object is sort of 'locked in'. If you want a useful, sensible, consistent interface, NOW you have to put the darn things on all the variables you expose. Because of this, finding a clever solution to avoid that one variable's malfunction is worth some effort. If you can eliminate the validation problem, 99% of the time you can avoid the getter-setter concept entirely and have much 'meatier' code where everything you see is doing something useful, rather than 5 pages of do-nothing.

[AbstractionAnon]

Lines 29-35: Assuming none of your getters modify your TextButton, all your getters should be const.

[jlb]

Since the class has no default constructor, a question you should be asking is can any of the variables passed to the constructor change after construction. If they remain static for the life of the class then the setters/getters would probably not be necessary. Also remember that the setters can be private if their only duty is to insure the constructor arguments are correct.

The code that I'm about to show you is just a header file, because I think that's all that's needed for this discussion, but what immediately jumped out at me when I looked at this header file is that the code seems over-engineered and like it has a lot of redundancies, but it also does look clean and nice, and it's easy to read.

Actually seeing the class implementation would probably help us better answer your questions.