How can i use getters and setters with m

I have a function in a game i'm working on that is the shop function and I need to do some math for calculating a total and subtracting money from the players account, but I'm having trouble, here is a snippet of the shop function:

while(amountChoice != -1)
                    {
                        cin >> amountChoice;

                        if(player.GetPlrMoney() >= amountChoice * itemPrice)
                        {
                            cout << "Are you sure you want to buy " << amountChoice * itemQuantity << " " << itemType << " of " << itemName << endl;
                            cout << "This will cost $" << amountChoice * itemPrice << endl;
                            cout << "Type in 1 for YES and 2 for NO" << endl;

                            cin >> confirmPurchase;

                            if(confirmPurchase == 1)
                            {
                                player.GetPlrMoney() -= amountChoice * itemPrice;

                                cout << "Thank you for your purchase: -$" << amountChoice * itemPrice << endl;
                                cout << "You recieved +" << amountChoice * itemQuantity << " " << itemName << endl;

                                //Add items to inventory
                                switch(menuChoice)
                                {
                                    case 1:{player.GetPlrHealthPacks() += amountChoice * itemQuantity;}break;
                                    case 2:{player.GetPistolAmmo() += amountChoice * itemQuantity;}break;
                                    case 3:{player.GetShotgunAmmo() += amountChoice * itemQuantity;}break;
                                    case 4:{player.GetRifleAmmo() += amountChoice * itemQuantity;}break;
                                    default: cout << "Error" << endl;
                                }

                                cout << "Your current money is $" << player.money << endl;
                                cout << "Current " << itemName << " is: ";

                                //show how many of chosen item the player has in backpack
                                switch(menuChoice)
                                {
                                    case 1:{cout << player.healthPacks << endl;}break;
                                    case 2:{cout << player.pistolAmmo << endl;}break;
                                    case 3:{cout << player.shotgunAmmo << endl;}break;
                                    case 4:{cout << player.rifleAmmo << endl;}break;
                                    default: cout << "Error" << endl;
                                }

                                cout << "\n";

                                menuChoice = 0;
                                amountChoice = 0;
                                confirmPurchase = 0;

                                save(player, dinosaur);
                                shop(player, dinosaur);
                            }
                            if(confirmPurchase == 2)
                            {
                                cout << "Ok what else would you like?\n" << endl;
                                menuChoice = 0;
                                amountChoice = 0;
                                confirmPurchase = 0;
                                break;
                            }
                        }break;
                    }

Here is my player class:

class Player
{
    public:
        Player(int PLAYERHEALTH, int PISTOLAMMO, int SHOTGUNAMMO, int RIFLEAMMO, int SCORE, int MONEY, int HEALTHPACKS, int TURNS, std::vector<int> ITEMS);

        void SetPlrHealth(int arg_plrHealth)
        {playerHealth = arg_plrHealth;}

        int GetPlrHealth()
        {return playerHealth;}


        void SetPistolAmmo(int arg_pistolAmmo)
        {pistolAmmo = arg_pistolAmmo;}

        int GetPistolAmmo()
        {return pistolAmmo;}


        void SetShotgunAmmo(int arg_shotgunAmmo)
        {shotgunAmmo = arg_shotgunAmmo;}

        int GetShotgunAmmo()
        {return shotgunAmmo;}


        void SetRifleAmmo(int arg_rifleAmmo)
        {rifleAmmo = arg_rifleAmmo;}

        int GetRifleAmmo()
        {return rifleAmmo;}


        void SetScore(int arg_score)
        {score = arg_score;}

        int GetScore()
        {return score;}


        void SetPlrMoney(int arg_plrMoney)
        {money = arg_plrMoney;}

        int GetPlrMoney()
        {return money;}


        void SetPlrHealthPacks(int arg_healthPacks)
        {healthPacks = arg_healthPacks;}

        int GetPlrHealthPacks()
        {return money;}

    private:
        int playerHealth;
        int pistolAmmo;
        int shotgunAmmo;
        int rifleAmmo;
        int score;
        int money;
        int healthPacks;
        int turns;
        std::vector<int> items;
};

The errors i get are:

C:\Users\thund_000\Desktop\Dinosaur Arena\main.cpp|543|error: lvalue required as left operand of assignment|

I was thinking i could do something like this:

1
2

money -= amountChoice * itemPrice;
 player.SetPlrMoney(money);

but i'm unsure if this is the best choice.

Last edited on

You've found one disadvantage of using getters and setters. Unless there is a specific reason why you need them, I'd get rid of them. It would certainly simplify your code.

http://en.wikipedia.org/wiki/Mediator_pattern
http://patterns.pl/mediator.html

Hi Chay,

case 1:{player.GetPlrHealthPacks() += amountChoice * itemQuantity;}break;

GetPlrHealthPacks() is a function call that returns a value, one can't assign a value to it.

Try assigning the return value to a variable, then use that variable in a calculation.

Just some other minor points:

When posting code, one can specify a firstline number: [ code firstline=543] So that code shown matches the compiler output. There is an article on this site about all the differnt things one can do with tags.

If you have more than 500 LOC in in main, consider using functions more.

A class with get / set functions for each member might as well be public. Using initialiser list in the constructor might eliminate the need for some of the set functions. Consider using a Mediator Design Pattern.

Hope all is well, and happy new Year :+)

Thanks for the tips, I'm still learning so Design Patterns are still too complicated for me right now. So if I shouldnt use getters and setters than what should i use? I was always taught to use them since data hiding is very important, I really dont see the use of a class if all of my variables are going to be public.

Last edited on

Make the members public. For many, many cases, data hiding is overrated. The purpose of the class is to encapsulate something. What else can a player do? The answers should lead to methods of the Player class.

JLBorges (13770)

Identify the class invariants for Player.
For instance:
a. score must be between 0 and 100.
b. rifleAmmo can't be negative.
etc.

Then, listen to what Stroustrup has to say (quoted by kemort in this post):
http://www.cplusplus.com/forum/beginner/142434/#msg751870
(Ensure that your setters take care not to violate the invariants)

If you are looking for unadulterated comic relief, read the whole of the linked thread.

Last edited on

Hi Chay,

Have a go at this idea:

Make a Base class CResource, along with Derived classes for individual resources such as cash, ammo etc.

For the CPlayer who is going to be using the resources, provide an interface function called AcquireResource which takes a smart pointer to CResource . This is very important to take advantage of polymorphism. When this function is called, the argument will be a smart pointer to whatever the object is - ammo say.

Now this AcquireResource can use the interface of the particular Resource to update the values inside CPlayer . The CResource class can have a protected bool AllowAcess variable (Inherited by derived classes), which dictates whether get functions should return a value or not. Each get function would check this variable before returning anything.

With the CSuperMarket scenario, the supermarket can maintain an CInventory which keeps track of how many resorces there are, and can allow items to be purchased via a CTransaction process, setting the AllowAcess to true , effectively signalling that the object has been bought, and the buyer can acccess the information.

I am at work, & have to do some stuff, so will post more info soon :+)

http://patterns.pl/mediator.html

Hi,

Am back in from the rain ....

Thanks for the tips, I'm still learning so Design Patterns are still too complicated for me right now.

Don't be afraid of complexity, instead adopt a "Divide & Conquer" approach. And start with something simple & gradually add complexity.

So, add an inventory to your player class. Make it a std::vector<CResource *>. (Using smart pointers)

Add the CResource* AcquireResource(CResource *TheResource); function .

Edit: I missed an argument - there needs to be somewhere to acquire the resource from - the Shop

Now add 1 derived resource class, say CAmmo.

class CAmmo : public CResource {};

Don't forget the AllowAccess member variable, and a function in the CShop that ~~a Player~~ the AcquireResource function can call to "Acquire" the object's info.

Hope this is enough to get you started.

Here is some code for a mediator pattern, just to show it's not that hard.

Last edited on

Note the overall point that TheIdeasMan and JLBorges are making: if you structure the code correctly, the design will make it impossible to make various errors. For example, pistol ammo, shotgun ammo and rifle ammo are just different items aren't they? And doesn't this point out that an item has both a type and a quantity?

Hi Chay,

I hope all is well a your end :+)

It's been a couple of days since your last reply (I know it's the weekend - you might have other stuff on), it's just that in the past (several times over the last 2 years), you have posted questions where you had probably written about 1000 LOC, and we have replied with completely different ideas that meant a complete rewrite of your code.

Now this is probably not good for your confidence, and I am hoping we all can work on this together to help you out. There are quite a number of people with vast amounts of knowledge & experience (much more than me) are happy to help, explain & suggest ideas.

So I am hoping we all can keep this topic going, work on things one step at a time, and come up with a minimal working system that you can then extend yourself later. At the end of this post, I am asking for some simple code, I sincerely hope that you are up for the challenge of learning some good stuff, and letting us all help you to do it. :+D

Further to dhayden's last post:

And doesn't this point out that an item has both a type and a quantity?

I would use a std::tuple for that, std::pair might have been OK, but we can later add things to a tuple. A tuple is just the same a record in a database, we can group things together without having to make a class for them.

Note that by ensuring a CPlayer has a vector of possessions of type CResource*, we have generalised it, meaning a CPlayer can possess any kind of resource, rather than just the ones hard coded into the class declaration in the OP.

I guess this points to a different way of thinking, some aspects of which would include at least the following:

1. Keep things general as much as you can - this avoids restrictions later;
2. Think about what objects an entity might have or use or is a type of;
3. What is the interface of the class going to look like?
4. When we have multiple classes, how are they going to interact? ;
5. Which classes need a knowledge of other classes? Which classes should be hidden from others? ;
6. What is the flow of information? What functions are we going to have to achieve that? ;
7. Can we make use of a design pattern?

I am sure that the experts on this site could add lots of things to this list . :+)

With number 2:

One might be tempted to create a CPerson class, with members m_FirstName, m_MiddleName, m_LastName, m_AddressLIne1, m_AddressLine2, m_State, m_PostCode, m_Country, m_DateOfBirth, m_DriversLicence etc.

But these could be made up of classes. For example the Names could at least be a std::vector<std::string> - people have more than 3 names. But if it was made a class, then one could have functionality of printing out middle initials, if required. Often people would rather not receive mail with all their names in full.

The Address could be a class too, that way if any other class needs an address - we don't have to reinvent the wheel there.

With dates, there is a problem of various formats: Is 03/09/2015 March 9th or 3rd September? Scott Meyers suggested creating a class for each month as well as a Date class. This helps take care of the days in each month & Leap year February.

Another quick example: A Shop doesn't have cash (we don't throw it on the floor), instead it has cash registers. We don't just put cash in a register, we have transactions which are also related to the Stock inventory. So a Shop can be a little more complicated than maybe first thought. But we can beak it down into it's components, and deal with them. However, we can still have a "dumb" version, as a starting point.

With number 3: The class interface.

Establish the invariants as JLBorges described.

If something can't be negative, does that give us a clue on what it's type should be - ie unsigned. Should there be a maximum limit? If the type was unsigned int , that would mean the player could have a very large amount of ammo. Should we change the type to something smaller, or should we have another const variable that is the limit? For example, in one of the TombRaider games, a limit was imposed for how many weapons could be carried (4 I think), and there was a limit to the number of grenades & Health Packs.

Think about what the object can do, and provide functions for those things. It is these functions that deal with the private member variables.

For example, if we were to talk geometry, and we had a CTriangle class. Btw, I wouldn't actually do that, but bear with me for a minute :+)

I wouldn't have these functions: MoveVertex1(double DeltaX, double DeltaY), MoveVertex2(double DeltaX, double DeltaY) , MoveVertex3(double DeltaX, double DeltaY) .

Instead I would have a Move(double DeltaX, double DeltaY) , this function would deal with the private variables. I wouldn't have variables Vertex1, Vertex2, Vertex3 either - I would have a vector of vertex's instead.

With different shapes, I wouldn't have CEquilateralTriangle, CSquare, CPentagon, CHexagon etc.

Instead, I would have CRegularPolygon, and CIrregularPolygon derived from CShape. An equilateral triangle is a regular polygon with 3 sides, other types of triangle are irregular polygons with 3 sides. In all the cases they are stored as a vector of points. A regular polygon can be defined by a centroid point, a radius (internal or external), and the number of sides. Hint: an n sided regular polygon can be made from n triangles with the centroid being common to them all.

The Move, Scale, Rotate, & Stretch function would exist in the CShape class, as they would be the same, no matter what the shape is. Try to push your functions as high up the inheritance tree as you can, so you only define them once. This is why it is important to generalise things as much as possible.

Well, that might be enough for now. I was just trying to give some ideas on how to look at things differently.

We (me & anyone else willing to help) look forward to seeing your new code. Just something simple at this stage: The CPlayer class with the vector of CResource* as a member, 1 derived resource class.

Hi, yes, I have been learning c++ for about 6 years now and the reason I am sttill stuck on the basics is because whenever I ask a question about something I always get conflicting answers and its very confusing trying to figure out who to listen to. I mean I get 5 people telling me one way is good, then 5 more telling me the exact opposite, it's very frustrating, then my confidence with programming gets low and i really dont want to do it for a while but hopefully we can figure it all out. I have a hard time with concentration so It was hard for me to read your entire post without distractions but I did get through it, but maybe i missed some stuff in it. I will re-read it and try to come up with some code for you, thank you for your help :)

Also I should add, this is a game I wrote 2 years ago and am just now coming back to. If you need to see the entire program I can put it on paste bin, I just didnt post it here cause it's around 1000 LOC.

Avilius (617)

If you have getters and setters on the same variable then you're generally doing it wrong.

What should I do then? just make them all public? I actually had that before i edited it a few days ago. Also, to be honest I would just like to keep things simple, I have a lot to learn still and i dont want to complicate it by learning something complex like design patterns. I will learn them, just not right now, I need to crawl before I can walk here.

Hi,

What I was proposing, was to start with something simple, then gradually add to it. We don't have to do design patterns, if we do that later, we can introduce them slowly with explanations.

Most of my last post was aimed at a different way of thinking, but for now just have a go at what I mentiond at the end.

There is no need to make all your members public, nor is there a need for lots of getters / setters IMO. The idea is to think about Actions (doing things) rather manipulating the member data . For example CurrentWeapon->Reload rather than getting / setting Ammo Levels with functions. The manipulation of these variables happens inside the Reload function.

Anyway my Lunchtime is over, so look forward to seeing your new code.

JLBorges (13770)

> What should I do then? just make them all public?

41. Make data members private, except in behaviorless aggregates (C-style structs)

Summary
They're none of your caller's business: Keep data members private. Only in the case of simple C-style struct types that aggregate a bunch of values but don't pretend to encapsulate or provide behavior, make all data members public. Avoid mixes of public and nonpublic data, which almost always signal a muddled design.

Discussion
Information hiding is key to good software engineering. Prefer making all data members private; private data is the best means that a class can use to preserve its invariants now, and to keep preserving them in the face of future changes.

Public data is bad if a class models an abstraction and must therefore maintain invariants. Having public data means that part of your class's state can vary uncontrollably, unpredictably, and asynchronously with the rest of its state. It means that an abstraction is sharing responsibility for maintaining one or more invariants with the unbounded set of all code that uses the abstraction, and that is obviously, fundamentally, and indefensibly flawed. Reject such designs outright.
...
Nonprivate data members are almost always inferior to even simple passthrough get/set functions, which allow for robust versioning.

Examples
Example 1: Proper encapsulation.
Most classes (e.g., Matrix, File, Date, BankAccount, Security) should have all private data members and expose adequate interfaces. Allowing calling code to manipulate their internals directly would directly work against the abstraction they provide and the invariants they must sustain.

Example 2: ...

Example 3: Getters and setters.
If there is no better domain abstraction available, public and protected data members (e.g., color) can at least be made private and hidden behind get and set functions (e.g., GetColor, SetColor); these provide a minimal abstraction and robust versioning.

Using functions raises the level of discourse about "color" from that of a concrete state to that of an abstract state that we are free to implement as we want: We can change to an internal color encoding other than int, add code to update the display when changing color, add instrumentation, and make many other changes without breaking calling code. ...

- Sutter and Alexandrescu in 'C++ Coding Standards: 101 Rules, Guidelines, and Best Practices'

Linked earlier:

There are several benefits to be obtained from restricting access to a data structure to an explicitly declared list of functions. For example, any error causing a Date to take on an illegal value (for example, December 36, 2016) must be caused by code in a member function. This implies that the first stage of debugging – localization – is completed before the program is even run. This is a special case of the general observation that any change to the behavior of the type Date can and must be effected by changes to its members. In particular, if we change the representation of a class, we need only change the member functions to take advantage of the new representation. User code directly depends only on the public interface and need not be rewritten (although it may need to be recompiled).
Another advantage is ...

- Stroustrup in 'The C++ Programming Language'

> I mean I get 5 people telling me one way is good, then 5 more telling me the exact opposite, it's very frustrating

That is what forums on the internet are: places where anyone can say anything.

When in doubt, think.

See what people you know for sure are knowledgeable about programming and C++ have to say about it. There is no dearth of material if you search for it; the great masters of C++ have not held back when it comes to teaching C++. It is extremely unlikely that Stroustrup, Koenig, Lippman or Alexandrescu would give you wrong advice.