patterncppMinor
Heroes of Might and Magic 3 battle simulator
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
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
And so in unitStack::attack, the appropriate line becomes
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
This will enable you to do things like:
and then can simplify some functions:
That's it for the moment. Hope this is useful.
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.