C++ object orientated issue

Question

I am trying to learn C++ OOP and I made the follwing code:

main.cpp

#include <iostream>
#include <string>
#include "monster.h"
using namespace std;

int main(int argc, char** argv) {
    Monster monster("Wizard",150,50);
    Monster monster2("Gorgoyle",450,15);

    cout << monster2.getHealth() << endl;
    monster.attack(monster2);
    cout << monster2.getHealth() << endl;
}

monster.h

#ifndef MONSTER_H
#define MONSTER_H
#include <iostream>
#include <string>
using namespace std;

class Monster
{
public:
    Monster(string name_, int health_, int damage_);
    ~Monster();
    int attack(Monster opponet);
    int getHealth();
    string name;
    int damage;
    int health = 0;
    int getDamage();
    void setHealth(int health_);
    void setDamage(int damage_);
    void setName(string name);
    void doDamageToOpponent(Monster opponent);
    string getName();
};

#endif

monster.cpp

#include "monster.h"

Monster::Monster(string name_, int health_, int damage_) {
    health = health_;
    setDamage(damage_);
    setName(name_);
}

Monster::~Monster() { }

int Monster::attack(Monster opponent) {
    doDamageToOpponent(opponent);
}

void Monster::doDamageToOpponent(Monster opponent) {
    int newHealth = opponent.getHealth() - this->getDamage();
    opponent.setHealth(newHealth);
}

int Monster::getHealth() {
    return health;
}

int Monster::getDamage() {
    return damage;
}

void Monster::setHealth(int health_) {
    health = health_;   
}

void Monster::setDamage(int damage_) {
    this->damage = damage_;
}

void Monster::setName(string name_) {
    this->name = name_;
}

string Monster::getName() {
    return name;
}

Now my problem is that, when I run this code I expect to have monster2 object to have 400 health left, but it is still 450 :S

What must be done here in order to to so? I noticed that it can be 400 in doDamageToOppoenet but when it leaves that block, then it is still 450. Please help me! Thanks.

Offtopic: You should probably consider `atackee.takeDamageFrom(atacker.getDamage())` method instead of `attacker.attack(attackee)`, this way you won't need to cope with atackee's guts in the attacker's methods (improving encapsulation and reducing coupling) and also will slightly reduce need of those accessors methods. This way you also won't need references or pointers, as `getDamage()` returns a value that you won't need to change. — Ivan Aksamentov - Drop, Sep 25 '14 at 19:12
Is that your [MCVE](https://stackoverflow.com/help/mcve)? Does not seem minimal to me! Also read [Why is `using namespace std;` considered bad practice?](http://stackoverflow.com/q/1452721) (especially relevant for headers). Also, headers should only include the minimum set of other headers neccesary to compile cleanly, no more and no less. — Deduplicator, Sep 25 '14 at 19:13
Also, oop is about encapsulation, which means abstracting away the (implementation-) details behind the public interface. You don't (and providing access to all members does not abstract them at all). — Deduplicator, Sep 25 '14 at 19:20

Marco A. · Accepted Answer · 2014-09-25T19:12:11.363

You're passing objects by value:

void Monster::doDamageToOpponent(Monster opponent) <- This should be by reference
int Monster::attack(Monster opponent) <- idem

that means: you're creating a new copy of the Monster object you meant to deal damage to in the functions you're calling, and then actually dealing that copy damage but leaving the original old object with the value untouched.

Signatures as follows would work instead:

void Monster::doDamageToOpponent(Monster& opponent)
int Monster::attack(Monster& opponent)

If you want to learn more about this, something to read on: Passing stuff by reference and Passing stuff by value

@user4080732 no problem, if that solved the problem remember to mark it as accepted (or anyone else's) — Marco A., Sep 25 '14 at 19:42

4pie0 · Answer 2 · 2014-09-25T19:25:10.020

The reason is that functions attack and doDamageToOpponent are taking copies of arguments, because you pass them by value. What happenes then is you change the copies of passed Monsters inside functions. After functions return, these copies die (as they are local to functions) and nothing happens to original, interested parties.

Try instead pass the argument by reference. Reference works as if it was the original variable. Consider:

int a = 0;
int &refa = a; /* refa acts as real "a", it refers to the same object "a" */
int b = a;     /* this is your case */
b = 6;         /* b will be changed, but "a" not */
refa = 6;      /* a is changed, really "a", refa is just different name for "a" */

Try:

int Monster::attack( Monster &opponent){
    doDamageToOpponent( opponent);
}

void Monster::doDamageToOpponent( Monster &opponent){
    int newHealth = opponent.getHealth() - this->getDamage();
    opponent.setHealth( newHealth);
}

score 1 · Answer 3 · answered Sep 25 '14 at 19:08

You are passing the opponent by value, i.e., the function:

int Monster::attack(Monster opponent);

will actually receive a copy of the opponent and modify that copy. Every time you have a function that modifies some object you need to pass the object to be modified by reference or pass a pointer to it, e.g.,

int Monster::attack(Monster& opponent);

or

int Monster::attack(Monster* opponent);

I recommend using const T& for input parameters and T* for output parameters, so in this case, the latter form. The reason why I recommend the latter for output parameters is because it makes it more explicit to the caller:

monster.attack(&monster2); // passing a pointer: monster2 will be modified.

C++ object orientated issue

main.cpp

monster.h

monster.cpp

3 Answers3