Pointers vs references for class objects

[Ch1156]

Ok so like this?:

#include <iostream>
#include <string>

using namespace std;

class Weapon
{
    public:
        Weapon(const string m_weaponName, const string m_weaponAmmoType, 
               const float m_weaponDamageDealt, const float m_weaponCost): weaponName(m_weaponName),
                                                                            weaponAmmoType(m_weaponAmmoType),
                                                                            weaponDamageDealt(m_weaponDamageDealt),
                                                                            weaponCost(m_weaponCost){}
        ~Weapon();
        
        //void WeaponSetup(string weapName, int weapDamage, string weapAmmo);
    
    private:
        const string weaponName;
        const string weaponAmmoType;
        const float weaponDamageDealt;
        const float weaponCost;
};

Weapon::~Weapon()
{
    
}

int main()
{
    Weapon("Handgun", "9mm", 5.0, 1.0);
    Weapon("Rifle", "5.56mm", 15.0, 4.0);

    return 0;
}

How do I display the info? like damage, name etc? It seems i still need a getter for each at least.

[Repeater]

It's more customary for the member variables to begin m_ , and for the parameters passed in to not begin m_

  public:
        Weapon(const string weaponName, const string weaponAmmoType, 
               const float weaponDamageDealt, const float weaponCost): m_weaponName(weaponName),
                                                                            m_weaponAmmoType(weaponAmmoType),
                                                                            m_weaponDamageDealt(weaponDamageDealt),
                                                                            m_weaponCost(weaponCost){}

     ~Weapon();
        
        //void WeaponSetup(string weapName, int weapDamage, string weapAmmo);
    
    private:
        const string m_weaponName;
        const string m_weaponAmmoType;
        const float m_weaponDamageDealt;
        const float m_weaponCost;

How do I display the info? like damage, name etc? It seems i still need a getter for each at least.

Sure, if there's a need for that then you need to add a getter. But as you go, consider your design and remember that a good class takes care of itself; if there is processing to be done that should be done by the class, then have the class do it rather than fetching member variables and doing it somewhere else.

You will note that now it is impossible to have an instance of Weapon class that is not fully initialised with values. You have removed an entire set of possible errors by ensuring that if your program contains an instance of a Weapon, it's ready to use. There is an anti-pattern I see a lot, which I think of as "the C style allocate-init", in which a class gets created in an uninitialised, bad, unusable state, and then a separate function (or even many functions) has to be called to prepare that object for use. This being C++, we have constructors and wherever possible we should ensure that if an object exists, it's good to go and is taking care of itself.

[Niccolo]

It's more customary for the member variables to begin m

@Ch1156

Read Stroustrup on this point.

Customary isn't really a good reason to use m_, even for members. Though I've never seen m_ precede input parameters.

There's no problem doing this, mind you - the m_ prefix dates to the "hungarian notation" of Microsoft, which is entirely discredited at this point (the notion....ok, may MS too).

[Ch1156]

Instead of writing getters I just made one function for displaying info about the weapons. I was wondering though, is there a way to retrieve the info without writing a getter at all? Like using a pointer or something? liek a simple function interface for getting the info of any weapon created and entered into it without having to write different functions for each piece of info?

#include <iostream>
#include <string>

using namespace std;

class Weapon
{
    public:
        Weapon(const string m_weaponName, const string m_weaponAmmoType, 
               const float m_weaponDamageDealt, const float m_weaponCost): weaponName(m_weaponName),
                                                                            weaponAmmoType(m_weaponAmmoType),
                                                                            weaponDamageDealt(m_weaponDamageDealt),
                                                                            weaponCost(m_weaponCost){}
        void DisplayWeaponInfo();
        
        ~Weapon();
    
    private:
        const string weaponName;
        const string weaponAmmoType;
        const float weaponDamageDealt;
        const float weaponCost;
};

Weapon::~Weapon()
{
    
}

void Weapon::DisplayWeaponInfo()
{
    cout << weaponName << endl;
    cout << weaponAmmoType << endl;
    cout << weaponCost << endl;
    cout << weaponDamageDealt << endl;
}

int main()
{
    Weapon handgun("Handgun", "9mm", 5.0, 300.0);
    
    handgun.DisplayWeaponInfo();
    
    Weapon rifle("Rifle", "5.56mm", 10, 600);
    
    rifle.DisplayWeaponInfo();

    return 0;
}

[Niccolo]

@Ch1156,

Sure, a "get" could get a collection of data, not just a single value.

I often chafe at the notion of getters and setters when there is no reason. If both get and set are provided, all protection may be logically removed, so the notion of making the values private mean nothing.

In the early days (90's), we were often "coached" to write getters and setters for everything "just in case".

That "just in case" turns out to be bad reasoning, by itself. If values require limiting or some other computational work, the setter at least has a purpose. The radian is a fair example. The radian (twice Pi) should be limited to a range, often -Pi to + Pi, but sometimes 0 to 2*Pi. For this, the setter has good reason.

Yet, long ago I realized the better logic was to establish a radian class. The purpose of the setter was then handled by the class itself. It occurred to me that fourth or fifth time I re-wrote the same setter for a double representing a radian, to which I finally recognized the purpose of objects wasn't being implemented by doing that repeatedly.

Sometimes the data in a class is actually public. Most often not, though. Your Weapon class does merit protection, but that doesn't automatically translate into a 1:1 correspondence of getters and setters for each individual member. That could be one function that gets or sets all (or several that are associated) in fewer functions.

[Ch1156]

Yeah I have written classes where most functions and variables were hidden and interacted with each other behind the scenes, but I cant find that project. What I want to do is create code that I can use to plug into stuff and focus more on writing actual code than setup stuff. For example, outputting constructor stuff, instead of writing a ton of cout statements, I want to just have one function I can call and plug the class object into the parameters and it outputs whatever is in there instead of me having to writ out everything over and over again, which is just good design in general.

So for my code how would I output the constructor contents without having to cout each private variable? is something like that even possible? Would it be possibel to select which thing I want outputted? I want to output select things but I dont want a bunch of getters cluttering up the interface, I would rather write a template function that does all the work and lets me get what I need to display.

[Repeater]

Customary isn't really a good reason to use m_, even for members.

I do find that a good reason (rather than just because it's customary) is to make it easy for me to look at a class function and know if a variable being used is a class member variable or not, without having to track it back to its origin. I understand some people use an IDE to the same effect, with colours or pop-ups or whatever. State is dangerous and knowing that some code is changing that state is very valuable.

[Ch1156]

I updated my code, this works pretty good but tell me if I can make it better:

I made the values enums so the user can tell what info they want to display instead of just entering in a meaningless number.

#include <iostream>
#include <string>

using namespace std;

enum weapVarNames{WEAPON_NAME = 1, WEAPON_AMMO_TYPE = 2, WEAPON_DAMAGE_DEALT = 3, WEAPON_COST = 4, ALL_INFO = 5};

class Weapon
{
    public:
        Weapon(const string m_weaponName, const string m_weaponAmmoType, 
               const float m_weaponDamageDealt, const float m_weaponCost): weaponName(m_weaponName),
                                                                            weaponAmmoType(m_weaponAmmoType),
                                                                            weaponDamageDealt(m_weaponDamageDealt),
                                                                            weaponCost(m_weaponCost){}
        void DisplayWeaponInfo(Weapon& weapon, int select);
        
        ~Weapon();
    
    private:
        const string weaponName;
        const string weaponAmmoType;
        const float weaponDamageDealt;
        const float weaponCost;
};

Weapon::~Weapon()
{
    
}

void Weapon::DisplayWeaponInfo(Weapon& weapon, int select)
{
    switch(select)
    {
        case WEAPON_NAME:
            cout << weaponName << endl;
            break;
        case WEAPON_AMMO_TYPE:
            cout << weaponName << " Ammo Type: " << weaponAmmoType << endl;
            break;
        case WEAPON_COST:
            cout << weaponName << " Cost: $" << weaponCost << endl;
            break;
        case WEAPON_DAMAGE_DEALT:
            cout << weaponName << " Damage: " << weaponDamageDealt << endl;
            break;
        case ALL_INFO:
            cout << weaponName << endl;
            cout << weaponName << " Ammo Type: " << weaponAmmoType << endl;
            cout << weaponName << " Cost: $" << weaponCost << endl;
            cout << weaponName << " Damage: " << weaponDamageDealt << endl;
            break;
        default:
            cout << "Could not display requested information" << endl;
    }
}

int main()
{
    Weapon handgun("Handgun", "9mm", 5.0, 300.0);
    
    handgun.DisplayWeaponInfo(handgun, WEAPON_DAMAGE_DEALT);
    
    cout << "\n";
    
    Weapon rifle("Rifle", "5.56mm", 10, 600);
    
    rifle.DisplayWeaponInfo(rifle, WEAPON_DAMAGE_DEALT);

    return 0;
}

[Repeater]

In this code, void Weapon::DisplayWeaponInfo(Weapon& weapon, int select)

This: Weapon& weapon

should not exist.

[Ch1156]

Ah, thanks, that was an oversight on my end, I fixed it. Lets say I have an enemy, I want the weapon to damage the enemy, but how would that happen without inheritance? how does the enemy know how much health to subtract based on the weapons damage?

[Repeater]

Well, many options. Up to you to design you code. Perhaps a function like this:

Enemy::sufferDamage(Weapon& weapon)
{
  m_health -= weapon.getDamage();
}

Or whatever code decides that the enemy is to suffer the damage does something:

if (enemy_suffers_damage_from_weapon)
{
  enemy.sufferDamage( weapon.getDamage() );
}

You think about what best fits your design and the right way for you will become apparent.

[Ch1156]

Ok so i hit a bit of a problem, so I have a player class and an enemy class, the player class attacks the dinosaur, so in the player attack class, it has the weapon and it subtracts fromt he dinosaurs health, but both are getters so its not working because I cant use -= or else i get a lvalue error:

void Player::Attack(Weapon& weapon,Enemy& enemy)
{
    enemy.GetEnemyHealth() -= weapon.GetWeaponDamage();
}

I dont really see why it wouldnt work, both are basically variables. I tried your first example in your post above and that worked, but this is a bit trickier.

[Repeater]

enemy.GetEnemyHealth()
What's this? It's a function that returns a number, yes? This function returns a number. The function DOES NOT give you any kind of handle to the actual member variable. You get back a number.
Let's say the current value is 7.

So it gets evaluated, and then this
enemy.GetEnemyHealth() -= weapon.GetWeaponDamage();
basically becomes this
7 -= weapon.GetWeaponDamage();

weapon.GetWeaponDamage()This gets evaluated to another number. Let's say it's 5.

So now this:
enemy.GetEnemyHealth() -= weapon.GetWeaponDamage();
is basically this:
7 -= 5;

Well that clearly makes no sense. How can you say that 7 is equal to 7 minus 5? How can we set the value of '7' to something else; that's not even a variable.

Perhaps you mean something like this:

void Player::Attack(Weapon& weapon,Enemy& enemy)
{
    // get current enemy health
    int currentEnemyHealth = enemy.GetEnemyHealth();

    // get weapon damage
    int weaponDamage =  weapon.GetWeaponDamage();

  // calculate new health
    int newHealth = currentEnemyHealth - weaponDamage;

   // now set the new health value
  enemy.setHealth(newHealth);
}

[Albatross]

P.S. to Repeater's comment: As this is C++, you also have the luxury of being able to make GetEnemyHealth return a reference to an int. In that case, the enemy.GetEnemyHealth() -= weapon.GetWeaponDamage(); bit would work as intended. It's not the greatest idea from a code design standpoint, though.

-Albatross

[Repeater]

Indeed. Once you start handing out references to class member variables, any such member variable could change at any time with zero warning or way to detect it happening. Nightmare scenario.


This is exactly what I found last month in my current employer's codebase; a GUI, handing out a reference to an internal ComboBox, allowing anyone to change the combobox selection and values at any time. Only found it when it caused a crash because one thread was trying to read from it while another thread was rewriting it.

[Ch1156]

Awesome, thanks! I thought of that but it seemed like a clunky way to do it so I decided against it. and yeah I would rather learn the right way and not learn stuff thats not good programming practice, no sense in learning something just to have to unlearn it later.

[Repeater]

You can take a shortcut:

void Player::Attack(Weapon& weapon,Enemy& enemy)
{
   enemy.setHealth(enemy.GetEnemyHealth() - weapon.GetWeaponDamage());
}

[Ch1156]

Is that ok to use? like is it good programming form?

Just curious because Albatross said:

enemy.GetEnemyHealth() -= weapon.GetWeaponDamage(); bit would work as intended. It's not the greatest idea from a code design standpoint, though.

But idk since it's part of a functions parameter list maybe its different than just trying to subtract the two outright.

[Albatross]

That's perfectly fine to use. What I was referring to as being a bad practice was making GetEnemyHealth() return a reference to that enemy's health variable. You wouldn't be directly changing the enemy's health with -= in Repeater's shortened example.

-Albatross

[Repeater]

This one:

int& enemy::GetEnemyHealth() 
{
  return m_health;
}

returns a reference. Not a simple number. A reference to the member variable. Anyone who has this reference can do whatever they like with the actual member variable. The reference is basically another name - an alias - for the actual member variable. So with that code, enemy.GetEnemyHealth() gives you a reference to the actual member variable. Effectively, enemy.GetEnemyHealth() IS the actual member variable.


This :

int enemy::GetEnemyHealth()  // note the difference
{
  return m_health;
}

does not return a reference. It returns just an int. A number. Whoever gets that has just a number, they do NOT have the actual member variable.