HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Heroes of Might and Magic 3 battle simulator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mightbattlemagicheroesandsimulator

Problem

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
#include
#include
#include

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(in

Solution

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.

Code Snippets

unitType::rawDamage() //if you do 3) above
{
    return (rand() % (m_maxDamage - m_minDamage+ 1)) + m_minDamage;
}
...
rawDamage += m_unitType.rawDamage();
class combatRound;
class unitStack
{ ....
    friend class combatRound;
}
unitStack& combatRound::fasterUnit()
{
    if ( m_s1.m_unitType.isFaster( m_s2.m_unitType ) )
    {
         return m_s1;
....
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() );
    }
}

Context

StackExchange Code Review Q#12543, answer score: 5

Revisions (0)

No revisions yet.