Return to Answer

deleted 15 characters in body

edited May 23, 2017 at 13:53

Loki Astari

• 97.7k
• 5
• 126
• 341

Inline Side-by-side Side-by-side Markdown

added 220 characters in body

edited May 23, 2017 at 9:39

Loki Astari

• 97.7k
• 5
• 126
• 341

Inline Side-by-side Side-by-side Markdown

answered May 23, 2017 at 9:31

Loki Astari

• 97.7k
• 5
• 126
• 341

##Summary

A lot of work still needs to be done on this.
Fix all the suggestions below then resubmit for further review.

###Dice Rolling.

if (diceType == 4) diceSide = round(rand() % (4));
if (diceType == 6) diceSide = round(rand() % (6));
...

Why not:

diceSide = rand() % diceType; // note `round()` not needed `%` is an int operation

###Random Number.

Using random like that does not give you equal probability.

The function rand() returns a number in the range [0..RAND_MAX]. If RAND_MAX is not an exact divisor of the number you use as the divisor after the % operator then you get an uneven distribution.

Example:
Say RAND_MAX is 32768
Say you are rolling a 6 sided dye.
Then probability of a:
 0: 5462/32768 // Notice 5462 (not 5461)
 1: 5462/32768 // Notice 5462 (not 5461)
 2: 5461/32768
 3: 5461/32768
 4: 5461/32768
 5: 5461/32768

To do this so you get an even distribution you need to throw away those extra two values.

int randMax = RAND_MAX / diceType * diceType;
int randVal;
while((randVal = rand()) > randMax) {
 // Do Nothing
}
diceSide =randVal % diceType;

###Modern Rand

The rand() and srand() are old ways of generating random numbers inherited from C. Modern C++ has its own (actually good) random number generators.

See <Random>

// Do this bit once.
std::default_random_engine generator;
// Do this bit when you want to get random number in a specific range.
std::uniform_int_distribution<int> distribution(1, diceType);
int dice_roll = distribution(generator);

###Class Design

Hmmmm.

class Player {
 public:
 //variable declaration
 string name;
 string classType;
 int strength, endurance;
 int id, DT = 20; //DT is dice type by the way
 //get players health
 int getHP() {
 return (10 + ((strength + endurance) * 2));
 }
 //get players hit
 int getHit() {
 return rollDice(DT);
 } //get damage 
};

Some basics.

1. Member Variables should be private
2. Don't use geter/seter methods (they break encapsulation).
3. One variable declaration per line.
4. Member variables should be initialized in the constructor.
5. Member methods should be verbs (ie actions).
   The action is something you can do to the object.
6. Don't write comments that say the same thing as the code.

###Static Const

If you have some variables used as const. Make them static members.

string name1 = "\nBruneor the ";
string name2 = "\nRichard the ";
string name3 = "\nFilbert the ";

Looks like these should be static const. They are valid for all members. You don't want to initialize them more than once and you don't look like you are going to modify them.

static const string name1 = "\nBruneor the ";
static const string name2 = "\nRichard the ";
static const string name3 = "\nFilbert the ";

###Switch

If you have a bunch of if statements that depend on the value of a single variable then a switch statement is probably better.

 if (randNum1 == 1) return name1;
 if (randNum1 == 2) return name2;
 if (randNum1 == 3) return name3;
 if (randNum1 == 4) return name4;

This would look better as:

 switch(randNum1)
 {
 case 1: return name1;
 case 2: return name2;
 case 3: return name3;
 case 4: return name4;

But an even better technique is to use an array.

static const std::string names[] = {"\nBruneor the ", 
 "\nRichard the ",
 "\nFilbert the ",
 .... etc
 };
return names[randNum1];

###Member Methods With Action

Going back to the member method that is an action:

int fightEnemy(Player &player, Enemy &enemy) {
 //declare all of these variables 
 int eHit = enemy.getHit();
 int pHit = player.getHit();
 int eHP = enemy.HP;
 int pHP = player.getHP();
 int playerLastRoll = pHit;
 int enemyLastRoll = eHit;

In this function you get information from the object. You do some calculation then I would expect you to put the result back. But you never update the object (which is weird). OK ignoring the weird part.

You can have a method that manipulates the state of the object when two players are fighting.

int fightEnemy(Player &player, Enemy &enemy)
{
 while(player.alive() && enemy.alive()) {
 player.attack(enemy);
 enemy.attack(player);
 }
}
void Attacker::attack(Attacker& victim)
{
 getPrimaryWeapon().swingAt(victim);
}

###Indentation

Fix the indentation. ITs important; people reading it need this so we can quickly spot the logical scope of operations. When the indentation is broken it makes the code hard to read.

###Using std

Stop doing this:

//so I dont have to type a million std prefixes
using namespace std;

Sure you have to add the prefix std::. But that's better than haing broken code. Read: Why is "using namespace std" considered bad practice?

The reason it is std and not standard is so that it is not a big burden to use the prefix.

###Add some constructors

This is way to verbose:

 Player yourname1;
 yourname1.name = "yourname1";
 yourname1.classType = "Scribe";
 yourname1.strength = 3;
 yourname1.endurance = 6;
 yourname1.id = 1;

If you add a constructor this becomes:

 Player yourname1("yourname1", "Scribe", 3, 6); // Id should be generated.