Basic duel fight with OOP

Question

How would you improve on this? It's been fairly long since I last did something slightly more extensive in C++, so I'm looking for ways to be more compatible with modern C++ ways, and this style of OOP. I think especially the user action part and new opponent generation might be what needs the most work?

main.cpp

#include <iostream>
#include "player.hpp"
#include "enemy.hpp"

int main() {
    Player pc;
    Enemy monster;

    while(pc.isAlive()) {
        // Player has the first round
        std::cout << pc.getName() << " attacks " << monster.getName() << "!" << std::endl;
        int dmg = pc.getAttackDmg();
        monster.takeDamage(dmg);

        // The monster strikes back unless dead
        if(monster.isAlive()) {
            std::cout << "Monster retaliates!" << std::endl;
            int monsterDmg = monster.getAttackDmg();
            pc.takeDamage(monsterDmg);
        } else {
            // Player receives a reward for slaying the enemy successfully!
            int experience = monster.getXP();
            std::cout << monster.getName() << " has died! You earn " << experience << " XP." << std::endl;
            std::cout << "-- Battle ends --\n" << std::endl;
            pc.addXP(experience);
            monster.getNew(pc.getLevel()); // Reset enemy and adjust to PC level
        }

        // You choose what to do.
        std::cout << "\nWhat is your action? (A)ttack / (H)eal / (R)un away and end the quest?";
        std::string pcChoice;
        std::cin >> pcChoice;
        if(pcChoice == "A" || pcChoice == "a") {
            continue;
        } else if(pcChoice == "H" || pcChoice == "h") {
            pc.heal();
        } else {
            break;
        }
    }

    return 0;
}

player.hpp

#ifndef PLAYER_H
#define PLAYER_H
#include "character.hpp"

class Player : public Character {
    private:
        int experience;
        void checkLevel();
    public:
        Player();
        void addXP(int n);
        int getLevel();
        void heal();
};

#endif

player.cpp

#include <iostream>
#include <string>
#include "player.hpp"

Player::Player() {
    attack = 5;
    defense = 20;
    health = 40;
    experience = 0;
    level = 1;
    std::cout << "Player enters! What is your name?" << std::endl;
    std::string pcName;
    std::cin >> pcName;
    name = pcName;
}

int Player::getLevel() {
    return level;
}

void Player::heal() {
    health += 20 * level;
}

// Whenever PC gets experience, we check for level-up
void Player::addXP(int n) {
    experience += n;
    checkLevel();
}

// XP needed for level-up increases according to PC level
void Player::checkLevel() {
    int targetExperience = 2000 * level;
    if(experience >= targetExperience) {
        level += 1;
        std::cout << name << " has leveled up to level " << level << "!" << std::endl;
        experience = 0;
        int attackBonus = level;
        int defenseBonus = level*2;
        int healthBonus = level*4;
        health = 40 + healthBonus;
    }
}

enemy.hpp

#ifndef ENEMY_H
#define ENEMY_H
#include "character.hpp"

class Enemy : public Character {
    private:
        int xp;
    public:
        Enemy();
        int getAttackDmg();
        int getXP();
        void getNew(int pcLevel);
};

#endif

enemy.cpp

#include <iostream>
#include "enemy.hpp"

Enemy::Enemy() {
    std::cout << "I\'m a monsterman!" << std::endl;
    name = "Monster";
    attack = 3;
    defense = 10;
    health = 30;
    xp = health * defense + attack;
}

// Monsters have a unique attack method
int Enemy::getAttackDmg() {
    return attack;
}

int Enemy::getXP() {
    return xp;
}

void Enemy::getNew(int pcLevel) {
    std::cout << "** GRAAARR!! ** A new monster appears!" << std::endl;
    attack = pcLevel * 3;
    defense = pcLevel * 10;
    health = pcLevel * 30;
    xp = health * defense + attack;
}

character.hpp

#ifndef CHARACTER_H
#define CHARACTER_H

class Character {
    protected:
        std::string name;
        int attack;
        int defense;
        int health;
        int level;
    public:
        Character();
        virtual bool isAlive();
        int getHealth();
        std::string getName();
        int getAttackDmg();
        int takeDamage(int d);
};

#endif

character.cpp

#include <iostream>
#include "character.hpp"

Character::Character() {
    name = "Dummy";
    attack = 1;
    defense = 1;
    health = 1;
    level = 1;
}

bool Character::isAlive() {
    return health > 0;
}

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

std::string Character::getName() {
    return name;
}

int Character::getAttackDmg() {
    int dmg = attack * level;
    return dmg;
}

int Character::takeDamage(int d) {
    int effectiveDamage = d - (defense/100);
    std::cout << name << " takes " << effectiveDamage << " DMG! ";
    health -= effectiveDamage;
    std::cout << "After the attack, " << health << " HP remains." << std::endl;
}

Answer 1

Here are some things that may help you improve your code:

Make sure non-void functions always return something

In your Character::take_Damage() function, it's declared as returning an int but nothing is actually returned. This is an error your compiler should be able to flag if you turn up the warning reporting. It's also not clear what it should return, so that might be addressed by a well-placed comment.

Eliminate unused variables and functions

Variables attackBonus and defenseBonus are decleared and assigned in Player::checkLevel() but never used otherwise. They should be omitted or the code expanded to actually use them (I'm assuming this was your eventual intention). Additionally, Character::getHealth() is never used and can also be omitted.

Fix the game logic

Right now, a very successful strategy is to simply keep using "Heal" and the player gets stronger and the monster suffers the same consequence as as if "Attack" had been specified. It's a surefire strategy, but a rather boring game and terribly unfair to the monsters!

Be careful with signed and unsigned

I'm guessing that experience should never be negative. Even the greenest warrior has minimally 0 experience, right? If that's the case, then experience in main() should be unsigned rather than int, otherwise at some point, using the strategy mentioned above, eventually you'll get a strange printout like this:

What is your action? (A)ttack / (H)eal / (R)un away and end the quest?h Edward attacks Monster! Monster takes 13113 DMG! After the attack, 1602 HP remains. Monster retaliates! Edward takes 8028 DMG! After the attack, 283696 HP remains.

What is your action? (A)ttack / (H)eal / (R)un away and end the quest?h Edward attacks Monster! Monster takes 13113 DMG! After the attack, -11511 HP remains. Monster has died! You earn -2146666468 XP. -- Battle ends --

Naturally, even if you change it to use unsigned you will still need to check for overflow, otherwise a very successful warrior will eventually be killed by his or her own success as the indomitable "numerical wraparound" monster claims another victim.

Try automatic testing

To find the result above, I didn't actually manually interact with the program that long. I automated the interaction using expect whith the following command script:

spawn ./duel
expect name?  {send Edward\n}
expect {
    "You earn -"  exit
    quest? {send h\n ; exp_continue}
}

You could use the same kind of strategy for doing more thorough testing of this program when you progress farther with it.

Consider separating I/O from program logic

It is often beneficial to separate the underlying game logic from the actual input and output involving interaction with the user. One good fit for a program like this would be the model-view-controller design pattern. This would also help with things like automated testing without the use of outside scripting.

Use a virtual destructor for base classes

The destructor of a base class of a should probably be declared virtual unless there is good reason not to. See this question for an explanation of why that's a good idea. In this case, I'd tell the compiler to write one or me:

virtual ~Character() = default;

Declare "overrideable" functions as virtual

The current code works just fine as it is, but if you ever want to have something like, say, a collection of monsters and a group of players, you will benefit by declaring over-ridden functions virtual. In this case, the Character::getAttack is redefined in the Enemy class and so should be declared as virtual in the base class. The reasoning is similar to the reason that one would make the destructor virtual. See this explanation of polymorphism to get a fuller explanation of why this is valuable.

Use const where practical

Functions such as getName() and getHealth() don't alter the underlying object, so they should be declared const. Also, you can avoid creating yet another copy of the name string by simply returning a const reference. The getName() function would look like this:

const std::string &Character::getName() const {
    return name;
}

Use constructor initialization lists rather than assignment

Generally speaking, using an initialization list is almost always preferable to using assignments within the body of a constructor. See this C++ FAQ answer for more details. It's usually faster, and faster is good. So instead of having something like this:

Character::Character()
{
    name = "Dummy";
    attack = 1;
    // ... etc.
}

use this instead:

Character::Character() :
    name("Dummy"),
    attack(1), 
    // ... etc.
{
}

Because they will be initialized in declaration order no matter what order they appear, it's good practice to write them in declaration order to prevent any confusion.

Use parameters for constructors

Because your classes set things to specific values anyway, it would make sense and simplify your code if you provide a constructor that takes parameters.

Character::Character(const std::string &nameVal, int attackVal, int defenseVal, int healthVal, int levelVal) :
    name(nameVal),
    attack(attackVal),
    defense(defenseVal),
    health(healthVal),
    level(levelVal)
{
}

Now the constructors for derived classes can use that constructor:

Enemy::Enemy() :
    Character("Monster", 3, 10, 30, 1),
    xp(health * defense + attack)
{
    std::cout << "I\'m a monsterman!" << std::endl;
}

If you delete the non-parameter constructor, you will assure that all values are actually initialized. This was not the case in the original code because the value for Enemy::level was never initialized.

Consolidate variables and functions in the base class

The base class includes level which is never used in Enemy, but doesn't include experience which is common to both Enemy and Player. It would probably make more sense to move experience to the Character class and to either move level to Player or to use it in Enemy in a similar way to how it's used in Player.

Consider renaming Enemy::getNew

Unlike a constructor, the Enemy::getNew isn't really returning a new Enemy instance, but just recycling the old one. With that in mind, it might be better to name it Enemy::reanimate to more accurately describe what the code is doing.

Don't Repeat Yourself (DRY)

Repeating the same code in multiple places generally leads to odd bugs and maintenance headaches. In this case, the calculations for the experience of the monster and other parameters appears in both the constructor and in getNew, so I'd recommend consolidating those into a private function. If we apply most of the changes I mentioned above and this once, the Enemy class implementation might look like this:

Enemy::Enemy() :
    Character("Monster", 1)
{
    setParams();
}
void Enemy::setParams() 
{
    attack = 3;
    defense = level * 10;
    health = level * 30;
    xp = health * defense + attack * level;
}
void Enemy::reanimate(int pcLevel) {
    level = pcLevel;
    setParams();
}

Reconsider the interface

In the current code, the only reason that takeDamage is called is because one character has attacked another. Further, the only reason to call getAttackDmg() is to get the value to pass to takeDamage. One could streamline the code a bit and perhaps make the interface a bit more evocative by combining the two into a different function:

int Character::takeDamageFrom(const Character &other) {
    int d = other.attack * other.level;
    int effectiveDamage = d - (defense/100);
    std::cout << name << " takes " << effectiveDamage << " DMG! ";
    health -= effectiveDamage;
    std::cout << "After the attack, " << health << " HP remains." << std::endl;
    return health;
}

Now the main code looks like this:

std::cout << "Monster retaliates!" << std::endl;
pc.takeDamageFrom(monster);

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

Answer 2

You could use initialization list in constructors, for example:

Character::Character()
:name{"Dummy"}
attack{1},
defense{1},
health{1},
level{1}
{}

I also used C++11 Brace-Initialization which is controversial but I like it.

Instead of returning large objects by value like this:

std::string getName();

use const ref

const std::string& getName();