0
\$\begingroup\$

I just started watching C++ videos couple of days ago. I tried taking everything I learned about classes and passing by references vs values. The main question I what would like to know is this the correct way to change the values of private class members or is there a more 'professional' way of doing it. (Or standard).

Entity.cpp:

void Entity::playerAttack(Entity &Player, Entity &Enemy) {
    cout << "\nPlayer is Attacking!" << endl;
    cout << "Enemy's health was at: " << Enemy.getHealth() << endl;
    totalHealth = Enemy.getHealth() - Player.getDamage();
    Enemy.setHealth(totalHealth);

    if (Enemy.getHealth() <= 0) {
        Enemy.setHealth(0);
        cout << "\nEnemies is 0 or below" << endl;
        cout << "Players health is at: " << Player.getHealth() << endl;
    }
    else {
        cout << "Enemies health is now at: " << Enemy.getHealth() << endl;
    }
}

void Entity::setHealth(float x) {
    health = x;
}

void Entity::setDamage(float x) {
    damage = x;
}

float Entity::getDamage() {
    return (damage);
}

float Entity::getHealth() {
    return (health);
}

Entity.h:

#pragma once
class Entity
{
public:
    void setHealth(float x);
    void setDamage(float x);
    void playerAttack(Entity &Player, Entity &Enemy);
    float getHealth();
    float getDamage();

    Entity();
    ~Entity();

private:
    float health;
    float damage;
    float healAbility;
    float totalHealth;
};

source.cpp:

Entity constructE(float damage, float health, float regen) {
    Entity Entity;
    Entity.setDamage(damage);
    Entity.setHealth(health);
    Entity.setRegen(regen);

    return (Entity);
}

int main() {
    Entity Player = constructE(10, 100, 10);
    Entity Monster = constructE(5, 20, 5);
    Player.playerAttack(Player, Monster);
}
\$\endgroup\$
  • \$\begingroup\$ return (damage); What's the purpose of the parens?? \$\endgroup\$ – πάντα ῥεῖ Apr 4 '17 at 22:04
  • \$\begingroup\$ Other programming languages require it so it's more of a habit and formatting style. \$\endgroup\$ – Mvpg Apr 4 '17 at 22:13
  • 1
    \$\begingroup\$ Have you tried to compile this, and does it compile, I see many things that should be flagged by the compiler as errors. \$\endgroup\$ – pacmaninbw Apr 4 '17 at 22:15
  • \$\begingroup\$ I have compiled it. \$\endgroup\$ – Mvpg Apr 4 '17 at 22:18
  • \$\begingroup\$ @Mvpg "I have compiled it." Sure? \$\endgroup\$ – πάντα ῥεῖ Apr 4 '17 at 23:00
5
\$\begingroup\$

I have a couple of issues with how you are doing this (it is very Java like not C++ like).

Constructors

After the constructor finishes your object should be in a valid consistent state. Having to call multiple "setters" to finish initialization is a big red flag.

Entity constructE(float damage, float health, float regen) {
    Entity Entity;
    Entity.setDamage(damage);
    Entity.setHealth(health);
    Entity.setRegen(regen);

    return (Entity);
}

Why not just have a "constructor" that takes these arguments.

Entity Player = constructE(10, 100, 10);

// Should look like:
Entity Player(10, 100, 10);

Passing an object to itself?

Player.playerAttack(Player, Monster);

You are passing player as an argument? As you are calling a method on the object "player" you already have access to the player (via this). So there is no need to pass the player here.

player.attack(monster);

Identifier conventions.

One thing I notice about your code is that everything starts with a capitol letter. This makes it hard to tell the difference between types and objects. One of the most important things in C++ is type.

So the usual convention is that "User Defined Types" have an initial capitol letter. While objects (variables) have an initial lower case letter.

player.attack(monster);
^ Object      ^ Object

While:

Player.attack(monster);
|             ^ object
^ Looks like a type.
  So attack looks likes a static member function (ie a class method).

Getters/Setters

Getters and Setters break encapsulation. They expose the internal implementation details of the class. If you see the pattern

 auto x = object.getValue();
 auto y = performSomeAction(x);
 object.setValue(y);

Then this is usually an indication that you have not designed your class well as you expose internal details to calculate a value. A better design would be to provide a method that performs an ACTION on the class.

 object.someAction();   // internally it can update its own state.

Re-Design

class Entity
{
public:
     Entity(int damage, int helath, int regen)
         : damage(damage)
         , health(health)
         , healAbility(regen)
    {}
    // Destructor not needed.

    // Check if an entity is alive.
    operator bool() const
    {
        return health > 0;
    }
private:
    // Internal use only by attack()
    int weaponDamage() const
    {
        return myRandomNumber() % 10;  // Damage in the range 0..9
    }

    // Take damage
    Entity& takeDamage(int amount)
    {
        // Can not go below zero.
        health = max(0, health - amount);
        return *this;
    }
public:

    // Attack an Enemy
    void attack(Entity &enemy)
    {
        std::cout << "Player:     " << *this
                  << "Attacking:  " << enemy;

        // Damage the enemy and check if he is alive in one line.
        if (enemy.takeDamage(weaponDamage()) {
            // Enemy Alive
            cout << "E. Status: " << enemy;
        }
        else {
            // Enemy Dead
            cout << "Enemies is Dead" << "\n";
            cout << "P: Status: " << *this;
        }
    }

    virtual std::ostream& print(std::ostream& str) const
    {
        return str << "health: "       << health 
                   << " damage: "      << damage
                   << " healAbility: " << healAbility
                   << "\n";
    }
    friend std::ostream& operator<<(std::ostream& str, Entity const& data)
    {
        return data.print(str);
    }
private:

private:
    float health;
    float damage;
    float healAbility;
};
\$\endgroup\$
  • \$\begingroup\$ I'd suggest noting that rand() isn't reliable and should be avoided. \$\endgroup\$ – Kodnot Apr 10 '17 at 16:11
  • \$\begingroup\$ @Kodnot: So would I (if we wanted soething really random or cryptographically secure). But that's not part of the point I was trying to make and distracts for the correct discussion. \$\endgroup\$ – Martin York Apr 10 '17 at 17:14
  • \$\begingroup\$ Although I do agree that proper randomness isn't really important here, I still believe that the subject should be informed. Ignoring such (small) mistakes / bad pieces of code can form bad habits, which are harder to get rid of later on. There is no need to give an in-depth explanation of the drawbacks of rand(), but I don't think that simply informing the subject that there are better alternatives would distract him from the correct discussion. (*Although now that I look at the code again, the subject didn't use randomness at all, so all this ranting of mine is pointless :P) \$\endgroup\$ – Kodnot Apr 12 '17 at 11:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.