7
\$\begingroup\$

I have written this basic Heroes of Might and Magic 3 Battle Simulator. I am decent at procedural, C-style code and I am trying to get better at Object Oriented Programming. (I know procedural code be better suited for this problem, but I am comfortable with this material.) Any and All critiques are appreciated (Speed Optimization, style, readability, maintainability, etc.). I am especially interested in comments on how i could improve the object oriented design and implement the Standard Library (or another library) better.

For those unfamiliar with the game, a quick example is given here. (although ignore the "If the Attack skill is lower, then damage is reduced by 2% per point of difference" bit because i believe the correct number is 2.5%).

#include <iostream>
#include <string>
#include <ctime>
#include <algorithm>

using std::cout;
using std::string;
using std::max;
using std::min;

//Global Variables
const double ATTACK_ADV_MULT = 0.05;    //View Ecoris's 2nd comment
const double DEFENSE_ADV_MULT = 0.0025; //http://heroescommunity.com/viewthread.php3?TID=11801&pagenumber=2
const double MAX_DMG_MULT = 4.0;
const double MIN_DMG_MULT = 0.3;
const int NUM_FIGHTS = 1000;

class unitStack
{
public:
    unitStack( string name, int speed, int attackSkill, int defenseSkill, int minDamage, 
        int maxDamage, int maxHealth, int curHealth, int numberOfUnits, int numberWins/*=0*/, double dmgMultiplier/*=0*/);
    string getName() const;
    int getSpeed() const;
    int getMaxHealth() const;
    int getCurHealth() const;
    int getNumberOfUnits() const;
    int getNumberWins() const;    
    int getDefenseSkill() const; 
    long int attack(unitStack);
    void takeDamage(long int);
    void addWin();
    void resetHealth();
    void resetNumUnits(int );
    void setDmgMultiplier(int);

private:
    string m_name;
    int getAttackSkill() const;  
    int getMinDamage() const;     
    int getMaxDamage() const;
    double getDmgMultiplier() const;
    void setHealth(int );
    void loseUnits(int);
    int m_speed;                //never changes
    int m_attackSkill;          //never changes
    int m_defenseSkill;         //never changes
    int m_minDamage;            //never changes
    int m_maxDamage;            //never changes
    int m_maxHealth;            //never changes
    int m_curHealth;            //fluctuates in combat, must be altered after damage/unit lost from stack
    int m_numberOfUnits;        //fluctuates during one fight, must be reset after each of the NUM_FIGHTS fights
    int m_numberWins;           // Starts at zero, can be increased up to NUM_FIGHTS
    double m_dmgMultiplier;     //Set Once
} ;

unitStack::unitStack( string name,
                      int speed,
                      int attackSkill,
                      int defenseSkill,
                      int minDamage,
                      int maxDamage,
                      int maxHealth,
                      int curHealth,
                      int numberOfUnits,
                      int numberWins=0,
                      double dmgMultiplier=0):
    m_name(name),
    m_speed(speed),
    m_attackSkill(attackSkill),
    m_defenseSkill(defenseSkill),
    m_minDamage(minDamage),
    m_maxDamage(maxDamage),
    m_maxHealth(maxHealth),
    m_curHealth(curHealth),
    m_numberOfUnits(numberOfUnits),
    m_numberWins(numberWins),
    m_dmgMultiplier(dmgMultiplier)
{
}

string unitStack::getName() const
{return m_name;}

int  unitStack::getSpeed() const
{return m_speed;}

int  unitStack::getAttackSkill() const
{return m_attackSkill;}

int unitStack::getDefenseSkill() const
{return m_defenseSkill;}

int unitStack::getMinDamage() const
{return m_minDamage;}

int unitStack::getMaxDamage() const
{return m_maxDamage;}

int unitStack::getMaxHealth() const
{return m_maxHealth;}

int unitStack::getCurHealth() const
{return m_curHealth;}

int unitStack::getNumberOfUnits() const
{return m_numberOfUnits;}

int unitStack::getNumberWins() const
{return m_numberWins;}

double unitStack::getDmgMultiplier() const
{return m_dmgMultiplier;}

long int unitStack::attack(unitStack Enemy)
{
    long int rawDamage = 0;
    long int damage = 0;
    for (int n=0; n <= this->getNumberOfUnits(); n++)
    {
        rawDamage += (rand() % (this->getMaxDamage() - this->getMinDamage() + 1)) + this->getMinDamage();
    }
    return damage = int(rawDamage * this->getDmgMultiplier());
}

void unitStack::takeDamage(long int dmg)
{
    if (dmg < this->getCurHealth())
    {
        this->setHealth(this->getCurHealth() - dmg);
    }
    else
    {
        //There must be a better way to do this
        dmg = dmg - this->getCurHealth(); //One unit gone from stack, top unit is at max health is at MaxHealth (reflected in next two lines)
        this->loseUnits( 1 + (dmg/this->getMaxHealth())); // Lose top stack unit, and then floor of dmg/MaxHealth more
        this->setHealth(this->getMaxHealth() - (dmg % this->getMaxHealth())); //Start at max (since top stackUnit is removed).
    }
}

void unitStack::loseUnits(int unitsLost)
{
    m_numberOfUnits -= unitsLost;
}

void unitStack::setHealth(int health)
{
    m_curHealth = health;
}

void unitStack::setDmgMultiplier(int enemyDefenseSkill)
{
    if (this->getAttackSkill() >= enemyDefenseSkill)
    {
        double dmgMultiplier = ((this->getAttackSkill() - enemyDefenseSkill) * ATTACK_ADV_MULT ) + 1;
        m_dmgMultiplier = min(dmgMultiplier, MAX_DMG_MULT);
    }
    else
    {
        double dmgMultiplier = 1 - ((enemyDefenseSkill - this->getAttackSkill()) * DEFENSE_ADV_MULT );
        m_dmgMultiplier = max(dmgMultiplier, MIN_DMG_MULT);
    }
}

void unitStack::addWin()
{
    m_numberWins += 1;
}

void unitStack::resetHealth()
{
    m_curHealth = m_maxHealth;
}

void unitStack::resetNumUnits(int units)
{
    m_numberOfUnits = units;
}

// Non-Class Prototypes
void greeting ();
void oneTurn (unitStack *, unitStack *);
void ReportResults (unitStack *, unitStack *);
int isUnit1Faster (int, int);
bool oneFight (unitStack *, unitStack *);
void CombatSim (unitStack *, unitStack *);

int main()
{
    greeting();
    srand(static_cast<unsigned int>(time(0)));

    //To-Do; add functionality so user can choose creature,numberOfUnits
    unitStack stack1("hobgoblin",7,5,3,1,2,5,5,219,0);
    unitStack stack2("centaur captain",8,6,3,2,3,10,10,100,0);
    unitStack * p_stack1 = &stack1;
    unitStack * p_stack2 = &stack2;

   cout << "\n\nSo, the matchup is " << stack1.getNumberOfUnits() << " " << stack1.getName() << "s versus "
        << stack2.getNumberOfUnits() << " " << stack2.getName() << "s\n\n";

    CombatSim (p_stack1, p_stack2);
    ReportResults (p_stack1, p_stack2);
    return 0;
}

void greeting()
{
    cout << "Welcome to the Heroes of Might and Magic III battle simulator!\n\n";
}

void ReportResults (unitStack * p_stack1, unitStack * p_stack2)
{
    cout << "The " << p_stack1->getNumberOfUnits() << " " << p_stack1->getName() << "s win " 
         <<  p_stack1->getNumberWins() << " times out of " << NUM_FIGHTS << "\n";
    cout << "The " << p_stack2->getNumberOfUnits() << " " << p_stack2->getName() << "s win "
         <<  p_stack2->getNumberWins() << " times out of " << NUM_FIGHTS << "\n";
    system("PAUSE");
}

//Faster unit Attacks.  If slower unit is still alive, CounterAttack.
void oneTurn (unitStack * p_stack1, unitStack  * p_stack2)
{
    if (isUnit1Faster(p_stack1->getSpeed(), p_stack2->getSpeed() ))
    {
        p_stack2->takeDamage(p_stack1->attack(*p_stack2));
        if (p_stack2->getNumberOfUnits() > 0)
        {
            p_stack1->takeDamage(p_stack2->attack( *p_stack1));
        }
    }
    else
    {
        p_stack1->takeDamage(p_stack2->attack(*p_stack1));
        if (p_stack1->getNumberOfUnits() > 0)
        {
            p_stack2->takeDamage(p_stack1->attack( *p_stack2));
        }
    }
}

int isUnit1Faster (int speed1, int speed2)
{
    if (speed1 > speed2)
    {
        return 1;
    }
    else if (speed2 > speed1)
    {
        return 0;
    }
    else
    {
        int coinFlip = rand() % 2;
        return coinFlip;
    }
}

bool oneFight (unitStack * p_stack1, unitStack * p_stack2)
{
    while (p_stack1->getNumberOfUnits() > 0 && p_stack2->getNumberOfUnits() > 0)
    {
        oneTurn (p_stack1, p_stack2);
    }
    if (p_stack1->getNumberOfUnits() > 0)
    {
        return 1;
    }
    else
    {
        return 0;
    }
}

void CombatSim (unitStack * p_stack1, unitStack * p_stack2)
{
    int maxNumUnits1 = p_stack1->getNumberOfUnits();
    int maxNumUnits2 = p_stack2->getNumberOfUnits();

    p_stack1->setDmgMultiplier(p_stack2->getDefenseSkill());
    p_stack2->setDmgMultiplier(p_stack1->getDefenseSkill());

    for (int i=0; i < NUM_FIGHTS; i++)
    {
        if (oneFight (p_stack1, p_stack2))
        {
            p_stack1->addWin();
        }
        else
        {
            p_stack2->addWin();
        }
        p_stack1->resetHealth();
        p_stack2->resetHealth();
        p_stack1->resetNumUnits(maxNumUnits1);
        p_stack2->resetNumUnits(maxNumUnits2);

        //To-Do, Add vector of ints to "unitStack", find mean, median, other stats for remaining units after win.
        //e.g. Centaur Captains won 996 out of 1000 matches.  On average, they had 55.56 units left.
        //(*p_stack1).numUnitsLeft[i] = tempStack1.numberOfUnits;
        //(*p_stack2).numUnitsLeft[i] = tempStack2.numberOfUnits;
    }
}
\$\endgroup\$

4 Answers 4

4
\$\begingroup\$

Must say I completely disagree with:

I know procedural code be better suited for this problem

Global mutable state is bad so I cringe when I see

//Global Variables

Fortunately all your global members are const so its not a real problem.

Don't passs by value (unless it is a small POD object). Pass by const reference to achieve the same result. If you need to mutate internally then you can make a copy at that point. SO

unitStack( string name,

// Prefer

unitStack(std::string const& name

Anything that never changes should be mareked as const:

int const  m_speed;                //never changes
//  ^^^^^

Same with methods and their parameters.

If you are exposing your state via geters:

int  unitStack::getSpeed() const
{return m_speed;}

int  unitStack::getAttackSkill() const
{return m_attackSkill;}

You are tightly coupling your code to where it is being used. This may or may not be a problem but it seems to be excessive here. Leave all your members private and don't expose them via getters. Provide methods that modify your objects state without exposing it.

Here again you are passing by value:

long int unitStack::attack(unitStack Enemy)

In this case it is probably not what you want. You are making a copy of Enemy that you are using. What you probably want is to pass a reference to the enemy. This way of you damage him you damage the real enemy not a copy.

long int unitStack::attack(unitStack& Enemy)
                               //  ^^^

Here is an example of a bad interface:

void unitStack::setDmgMultiplier(int enemyDefenseSkill)

To use this you need to know enemyDefenseSkill to get that you need to extract (or get) state from the enemy. Which exposes your code to being abused. Better to pass a reference to the enemy and let you and the enemy talk directly.

void unitStack::setDmgMultiplier(UnitStack const& enemy)
{
    // Becuase your a unitStack object you can accesses other unitStack objects.
    // so the this is valid here:

    if (m_defenseSkill < enemy.m_defenseSkill)
    {
          // STUFF
    }

Prefer to pass by reference than pointer:

void oneTurn (unitStack *, unitStack *);

Here you need to validate that the object exists before you can use them. If you pass by reference you know they exist.

void oneTurn (unitStack& , unitStack& );

Note: int C++ the * and the & are usually placed by the type rather than the identifier. Not a big thing and their are people that still do it your way.

Passing pointers around in C++ is practically never done. This is because you don't know who owns the pointer and ownership is a big thing. It is the responsibility of the owner to delete the object. So it is very rare (and only deep in the bowels of a class to see a RAW pointer). Normally references work otherwise you should look into smart pointers.

Now you don't need to do this:

unitStack stack1("hobgoblin",7,5,3,1,2,5,5,219,0);
unitStack stack2("centaur captain",8,6,3,2,3,10,10,100,0);
unitStack * p_stack1 = &stack1;
unitStack * p_stack2 = &stack2;

CombatSim (p_stack1, p_stack2);

// You could have simplified that a lot with:
unitStack stack1("hobgoblin",7,5,3,1,2,5,5,219,0);
unitStack stack2("centaur captain",8,6,3,2,3,10,10,100,0);

CombatSim (&stack1, &stack2);

// But now that you know about references it can be:

CombatSim (stack1, stack2);

Looking at all these free functions:

void oneTurn (unitStack *, unitStack *);
void ReportResults (unitStack *, unitStack *);
bool oneFight (unitStack *, unitStack *);
void CombatSim (unitStack *, unitStack *);

I would encapsulate them in a Tournament manager class. The tournament takes two combatents as parameters in the constructor (passed by reference). Then You have one method called commense() the others are private (and you don't need to pass the anything around (as they are members of the Tournament).

\$\endgroup\$
2
  • \$\begingroup\$ Thank you so much, this is great! I definitely will change the pointers to references, stop passing by value and make a Tournament class. One Question, are global consts poor practice? would it be better to put 0.05 directly into the code rather than using ATTACK_ADV_MULT? or would it only be ill advised if they were mutable? (i.e. not const) \$\endgroup\$
    – k_Dank
    Jun 21, 2012 at 20:35
  • \$\begingroup\$ If a constant is only used in one place (as an argument) then I see little point in making it a const variable just use it in that one place. \$\endgroup\$ Jun 21, 2012 at 21:19
5
\$\begingroup\$

A few thoughts:

1) I dislike using declarations, even if you're specific (as here) unless you're using them very often - the minor clutter the namespace produces is worth it IMHO.

2) Don't use this->. It's implied and usually produces too much clutter (e.g. takeDamage).

3) For the constant parts of unitStack, separate them out into a unitType structure - it groups them together into something that's standalone.

4) For any function, if you're passing classes, then pass a reference instead - it avoids an unnecessary copy. And for that matter, if you're passing pointers then consider references instead as well, unless NULL is a perfectly valid value.

5) The number of "get..." functions you have is a problem, I think. Bring their use into a suitable class function. So you might have

unitType::rawDamage() //if you do 3) above
{
    return (rand() % (m_maxDamage - m_minDamage+ 1)) + m_minDamage;
}
...

And so in unitStack::attack, the appropriate line becomes

rawDamage += m_unitType.rawDamage();

and remove getMinDamage() & getMaxDamage() (and where does Enemy get used in this function?).

6) Be consistent in your naming convention. Don't have some variables beginning with capitals and some with small letters (e.g., dmg & Enemy) (I suspect it's a mistake, but still!).

7) If a function returns a bool, return true and false, not 1 and 0.

8) If function uses both stack1 and stack2, consider introducing a "combatRound" class with the combatants as members. Then have it as a friend of the unitStack class

class combatRound;
class unitStack
{ ....
    friend class combatRound;
}

This will enable you to do things like:

unitStack& combatRound::fasterUnit()
{
    if ( m_s1.m_unitType.isFaster( m_s2.m_unitType ) )
    {
         return m_s1;
....

and then can simplify some functions:

combatRound::oneTurn() // although you need a better name!
{
    unitStack& attacker = fasterUnit();
    unitStack& defender = slowerUnit();
    defender.takeDamage( attacker.attack() );
    if ( defender.m_numberOfUnits > 0)
    {
        attacker.takeDamage( defender.attack() );
    }
}

That's it for the moment. Hope this is useful.

\$\endgroup\$
0
5
\$\begingroup\$

I've been thinking this over for a few days. After thinking about it for a while, I think I'd make a fundamental change in how you're dealing with the health in a "stack".

At least as you seem to be using it, a "stack" of basically just represents a "pool" of health points. For example, if you have a stack of 50 Xs, that basically means you have 50 times as much health as if you only had one. An individual unit, however, doesn't really seem to have much meaning beyond the health points it contributes to the stack.

Assuming that's correct, I think I'd structure the "stack" a bit differently -- specifically, instead of having a number of units with a specific amount of health apiece, I would (at construction time) convert the number of units and health points per unit into a single pool of health points. At the conclusion of the battle, convert the remaining health on the winning team into the number of units remaining.

This simplifies what you store for a stack, and (especially) the computation for "takeDamage". At construction you do something like health_pool = max_health * num_units; and takeDamage simplifies to something like: health_pool -= dmg; Since you can now take an arbitrary amount of damage at a time, you can eliminate loseUnits entirely (it becomes a side-effect of takeDamage -- if you damage you take exceeds the health of an individual, then you lose some units).

\$\endgroup\$
1
  • \$\begingroup\$ Yeah, this could definitely work. the number of units in a stack only is used for two things; 1. determining if unitStack is dead, 2. generating damage. Both these things could de done using a health pool. Thank you! \$\endgroup\$
    – k_Dank
    Jun 21, 2012 at 20:25
4
\$\begingroup\$

Adding to what has already been mentioned by others.

  1. You included only the items that you need from the std namespace. This is a good practice.

    using std::cout;
    ...
    
  2. In general, a class declaration goes in its own header file (.h or .hpp) and its implementation goes into a .cpp file.

  3. Using const global values is a good way of storing data. But as this tiny game program evolves into something more serious, you would realize that it is pragmatic to make these values configurable. For instance, NUM_FIGHTS need not be 1000 all the time but would be better off being a dynamic value. So, you can think of reading all such metadata from a configuration file rather than hard-coding it in your program.

    const int NUM_FIGHTS = 1000;
    ...
    
  4. If a variable can take only specific values, it might be OK here to just mention this in a comment. But in a large project where several developers are involved, it is a sure-shot way of adding bugs to the code. For example:

    int m_numberWins;           // Starts at zero, can be increased up to NUM_FIGHTS
    

    This restriction needs to be enforced via code:

    void unitStack::addWin()
    {
        if (m_numberWins < NUM_FIGHTS)
            m_numberWins += 1;
    }
    

    You can also enforce the restriction via assertions (which approach to use? That should be the topic for another post). Same rule applies to setHealth() since health should never exceed m_maxHealth.

  5. You might like to start thinking about programming defensively when it comes to larger projects.. For example, if the unitsLost value passed to loseUnits() is more than m_numberOfUnits, the resulting value of m_numberOfUnits will be negative (which does not make much sense in a game of Heroes of Might and Magic 3). Moral of the story: trust no one. Not even yourself. Make sure that all assumptions and boundary conditions are checked and enforced in the code. It would be advisable to read more about Design By Contract methodology in this context.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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