   [C++] 'Battle' problems
  C++ using VS 2005Far from finished, but I have run into a problem.  Also critique my newb code Problem: When attack (The only function other than quit that works) the player and enemy will have the same amount of attack points against each other, ieI hit attack and hit him for 2 points, he attacks me for 2 also.  I attack him again, I hit him for 4 points and he hits me for 4.. etc.The code where this is stored is the attack() functions for player and enemycode:#include <iostream>#include <ctime>using namespace std;class player{public:player();~player();int attack();int menuChoice;int attackPoints;};player::player(){}player::~player(){}int player::attack(){srand(time(0));player::attackPoints = rand() % 5 + 1;return player::attackPoints;}class enemy{public:enemy();~enemy();int enemyAttack();int enemyattackPoints;};enemy::enemy(){}enemy::~enemy(){}int enemy::enemyAttack(){srand(time(0));enemy::enemyattackPoints = rand() % 5 + 1;return enemy::enemyattackPoints;}int main(){player playerInfo;enemy enemyInfo;int playerHealth = 20;int enemyHealth = 20;while ((playerHealth > 0) || (enemyHealth > 0)){cout << "What would you like to do?" << endl;cout << "1 Attack" << endl;cout << "2 Heal Self" << endl;cout << "3 Quit" << endl;cin >> playerInfo.menuChoice;switch(playerInfo.menuChoice){case 1:cout << "You attack the enemy for " << playerInfo.attack() << " hits!" << endl;enemyHealth = enemyHealth - playerInfo.attackPoints;cout << "He now has " << enemyHealth << " health left" << endl << endl << endl;break;case 2:break;case 3:break;default:cout << "Incorrect choice" << endl;break;}cout << "He attacks you for " << enemyInfo.enemyAttack() << " hits!" << endl;playerHealth = playerHealth - enemyInfo.enemyattackPoints;cout << "You know have " << playerHealth << " health left!" << endl << endl << endl;}}also please critique my code, it is far from done though.   
  Do not call srand() in your program more than once. Definitely do not call it before every random number.   ;Quote:Original post by SiCraneDo not call srand() in your program more than once. Definitely do not call it before every random number.Once again thanks SiCrane, I'll get back onto working on the code.   ;Quote:Original post by PowellAlso critique my newb code No problem [wink]First, some minor issues. Don't write empty constructors and destructors. They do nothing. Simply omit them.However, if you have members it is a good idea to initialise them. So, I would recommend omitting the destructors and filling out the constructor by choosing a good starting value for each member variable. Don't forget to use initialiser lists in the constructor (google the term if you haven't come across it yet).Second, when in a member function, you don't need to qualify member variables with the class name. Put more simply, player::attack could be implemented like this:int player::attack(){// omitted as you now understand: srand(time(0));attackPoints = rand() % 5 + 1;return attackPoints;}Next, the health of the player/enemy is surely a good candidate for a member?While we are discussing what is a suitable member, should we be storing "attackPoints"? After all, its value is only used in player::attack, and it is returned from there. With a little bit of work inside main(), it could be made a local:int player::attack(){int attackPoints = rand() % 5 + 1;return attackPoints;}Or more concisely:int player::attack(){return rand() % 5 + 1;}The "menu choice" is a poor member variable, because it is used only inside main(), and is only tangentially related to the player. I would simply make it a local there. This is a recurring theme: temporary values (such as "choice", and "damage for a single attack") should be locals. Members are reserved for long term state (such as "health").Next, lets look at it from an OO view. We have enemies, and players. However, apart from having different variable names, the code looks remarkably similar. We can take advantage of this to reduce the repetition. We create a single class, maybe called "Character", and make enemy and player both instances of it.This next snippet will show the above changes. I introduced a few additional things, not major but to show some of the flexibility. The main thing is that a character can have a variable starting health, and a variable damage rate. So a rat monster might have health and damage values or 2 or 3, while a bigger creature will have stats similar to the player itself.#include <string>class Character{public:    Character(const std::string &name, int health, int attack);    void attack(Character &other);    bool alive() const;private:    std::string name;    int heath;    int attack;};void Character::attack(Character &other){    int damage = rand() % attack + 1;    std::cout << name << " attacks " << other.name << " for " << damage << " points\n";    other.health -= damage;    if(other.alive())    {   std::cout << other.name << " now has " << other.health << " health remaining\n";    }    else    {   std::cout << other.name << " is now dead\n";    }}int main(){    // You can read in the players name, or whatever you want.    Character player("Doug", 20, 5);    Character enemy("The Spider Monster", 20, 5);    while(player.alive() && enemy.alive())    {   // use "\n" instead of std::endl where possible.   // std::endl flushes buffers, which is usually unnecessary   cout << "What would you like to do?\n";cout << "1 Attack\n";   cout << "2 Heal Self\n";   cout << "3 Quit\n";    // Notice how I declare the variable only when necessary   // This is a good practise.   // Note that this isn't possible for "enemy", because the enemy   // needs to retain its state (i.e. its health) between loops.   int choice;   cin >> choice;   switch(choice){case 1:player.attack(enemy);break;case 2:    break;case 3:    break;default:    cout << "Incorrect choice" << endl;    break;}  enemy.attack(player);    }    if(player.alive())    {   cout << "You defeated your foe :)";    }    else if(!enemy.alive())    {cout << "You died heroically :|";    }    else    {    cout << "You died for nothing :(";    }}This doesn't keep the exact logic of your program, but it is close enough. The implementation of the constructor and the "alive" member function are left as an excersize (translation: I'm lazy [smile]).   ;  Thanks ripoff for the help :DChanging my code as needed   