Basic duel fight with OOP

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 
#include "player.hpp"
#include "enemy.hpp"

int main() {
    Player pc;
    Enemy monster;

    while(pc.isAlive()) {
        // Player has the first round
        std::cout > 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 
#include 
#include "player.hpp"

Player::Player() {
    attack = 5;
    defense = 20;
    health = 40;
    experience = 0;
    level = 1;
    std::cout > 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

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

StackExchange Code Review Q#77760, answer score: 6