Are getters and setters fine to use for classes?

Pages: 12
I have a class and i was wondering if it looks ok? I want to make sure im doing it "right", i know there is no right way to make a class but still does it look okm can you give me any pointers (haha get it? -.- bad c++ joke) anyways ya i heard that getters and setters are bad but idk.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
#ifndef PLAYER_H
#define PLAYER_H

class player
{
    private:
        int health;
        int money;
        int score;

    public:
        void conditions();
        void save();
        void load();
        player();
        ~player();

        void set_health(int h);
        void set_money(int m);
        void set_score(int s);

        int get_health();
        int get_money();
        int get_score();
};

player::player()
{
    health = 100;
    money = 35;
    score = 0;
}

player::~player()
{

}

//Setters

void player::set_health(int h)
{
    health = h;
}

void player::set_money(int m)
{
    money = m;
}

void player::set_score(int s)
{
    score = s;
}

//Getters

int player::get_health()
{
    return health;
}

int player::get_money()
{
    return money;
}

int player::get_score()
{
    return score;
}

#endif // PLAYER_H
It's fine. You can omit the destructor in the same way you've omitted the copy constructor and assignment operator. The defaults do the right thing in this case, as the class doesn't have pointers.

But it's fine the way it is.
In your example, I would just have the data members public, and not write useless code (by the way your destructor is useless code too).

If you want to restrict what data the user can see, use getters.
If you want to restrict what data the user can change and how, use setters.

But if you're just giving the user all the data, and also let him change the data to anything he wants (without even checking if data is OK), then your setters and getters are useless code. Unless you plan to improve them in the future. (Which you won't.)
No, no, no. Never use public data in a class for any of its uses, whether an abstract data type or an object, and of course an interface won't have data.

Remember a record (struct) is just a collection of fields and that's different in principle.
And by the way. If you're implementing the functions in the header file, you had better either define them in the class body or mark them as inline, otherwise you'll run into linker errors sometime later.
Functions

int get_health();
int get_money();
int get_score();

should be defined as

int get_health() const;
int get_money() const;
int get_score() const;

As for getters and setters in whole then you are doing in "right way".:)

Last edited on
i heard that getters and setters are bad but idk

getters and setters are bad, but not just because they are useless code. It's because they ruin object-oriented design.

Let's look at health, for example: you have a get_health(), which is fine (except that it should be const, of course), if it makes sense for someone else to be able to quanitfy how healthy a player is just by looking at him, but what is set_health() supposed to mean?

So you walk around the world and suddenly some external force comes in and makes your health a totally different number? It's a lot more likely that your health will change, as a consequence of getting hurt or resting up instead. Resting is something a player can do, setting the health is not.

Besides failing to model the object behavior, setters introduce tight coupling, because the logic that belongs to your class now sits elsewhere: if you bump into a tree, the tree has to communicate your new health value to you, and for that to happen, the tree has to read your current health value, real your other attributes (toughness, etc) factor in your speed and angle and whatever other variables are needed, and send back your new health value. The tree now knows all about the player. Even if you factor out your health change calculations to some sort of GameMechanics class, it's still never going to make sense to have a public member function called "set_health()" on a player.

PS: do try to use proper syntax for constructors, it's
1
2
3
4
5
player::player()
:   health(100),
    money(35),
    score(0)
{ }


PS: of course there are classes where setters make sense: std::vector has a setter called resize(), for example, because resizing is what vectors do. Note that it doesn't just change one member variable, though.
Last edited on
Ok thanks for all the help guys but could you help me get rid of my getters and setters and help me do it the "right" way? I need to see the code displayed in the way you guys are telling me as im a visual learner.

This is a clas im using for a text turn based battle game.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
#ifndef PLAYER_H
#define PLAYER_H


class player
{
    private:
        int health;
        int pistolAmmo;
        int shotgunAmmo;
        int rifleAmmo;
        int score;
        int money;

    public:
        player();
        ~player();

        void set_health(int H);
        void set_pistolAmmo(int PA);
        void set_shotgunAmmo(int SA);
        void set_rifleAmmo(int RA);
        void set_score(int S);
        void set_money(int M);

        int get_health();
        int get_pistolAmmo();
        int get_shotgunAmmo();
        int get_rifleAmmo();
        int get_score();
        int get_money();
};

//Doing what Cubbi said here in the constructor
player::player():
    health(100),
    pistolAmmo(17),
    shotgunAmmo(8),
    rifleAmmo(30),
    score(0),
    money(0)
{}


player::~player()
{

}

void player::set_health(int H)
{
    health = H;
}

void player::set_pistolAmmo(int PA)
{
    pistolAmmo = PA;
}

void player::set_shotgunAmmo(int SA)
{
    shotgunAmmo = SA;
}

void player::set_rifleAmmo(int RA)
{
    rifleAmmo = RA;
}

void player::set_score(int S)
{
    score = S;
}

void player::set_money(int M)
{
    money = M;
}

int player::get_health()
{
    return health;
}

int player::get_pistolAmmo()
{
    return pistolAmmo;
}

int player::get_shotgunAmmo()
{
    return shotgunAmmo;
}

int player::get_rifleAmmo()
{
    return rifleAmmo;
}

int player::get_score()
{
    return score;
}

int player::get_money()
{
    return money;
}

#endif // PLAYER_H 
Last edited on
closed account (zb0S216C)
Perhaps making use of interfaces and inheritance:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
struct ICollidableObject
{
    virtual ~ICollidableObject( ) { }
    virtual signed int QueryDamage( ) const = 0;
};

struct CollidableObject : ICollidableObject
{
    private:
        signed int Damage_;

    public:
        signed int QueryDamage( ) const
        { return( this->Damage_ ); }
};

struct IActor
{
    virtual ~IActor( ) { }
    virtual void Kill( ) = 0;
    virtual void Resurrect( ) = 0;
};

class Player : IActor
{
    private:
        signed int Health_;

    public:
        void Kill( );
        void Resurrect( );
        void OnObjectCollision( ICollidableObject *Object_ )
        {
            this->Health_ -= Object_->QueryDamage( );
            if( this->Health_ < 0 )
                this->Kill( );
        }
};

Not the best code here, but it's a better approach.

Wazzak
Last edited on
Framework im sorry i dont understand your code is there an easier way? i dont understand pointers of any kind, or virtual.
My interpretation of Cubbi's post is that this is to be avoided:
1
2
3
4
5
6
7
8
9
void player::set_rifleAmmo(int RA)
{
    rifleAmmo = RA;
}

void player::set_money(int M)
{
    money = M;
}


Where a better approach may be:
1
2
3
4
5
6
7
8
bool player::buyRifleAmount(int amount, double cost)
{
  if (money < amount * cost) return false;

  rifleAmm0 += amount;
  this->money -= (amount * cost);  // how to use "this"
  return true;
}


The point that I see is that it isn't the caller's job to figure out if the player has enough money to buy the ammo.
Last edited on
why do i need to make the money and rifle ammo functions together like that? what if i just wanted to do just the rifle function?
Getters and setters are almost always good. Never make your member data public. NEVER!

However, a well-designed API may warrant a higher level of abstraction, as Cubbi had pointed out.

And always maintain const correctness, as Vlad had suggested.
> Never make your member data public. NEVER!
¿do you understand why?
¿how is giving accessors for all the members different from making them public in the first place?
Well, it does provide you with a shallow layer of indirection if you need to change the internal representation. Weak because a change in a fragile design like that will probably require refactoring the interface sooner or later anyway, but not entirely without merit. =P
Here's an idea, make the getters and setters private.
And then write public accessors for those functions...
"Here's an idea, make the getters and setters private."

"And then write public accessors for those functions..."

Can i have examples of these? I know how to make the setters and getters private thats easy but what do i do to access all of them?
closed account (zb0S216C)
Make some friends. If a class has a friend, that friend can access public, private and protected members of the class. Of course, placing these strategically can enhance encapsulation but can also destroy it.

Wazzak
ok can i have an example?
Pages: 12