Will you tell me how badly I wrote small battle-game?

[KruSuPhy]

Hi, I recently picked back up on C++(and by recently, I mean this morning) after not looking at it for a while. I've never known very much C++, but I've tried to learn before and gave up. However, I'm back and interested in actually learning this time.
To try and show myself what I've learned, I made a little battle game. And by little, I mean really little. Literally all you do is attack your enemy until either you or him is dead. However, I thought it would be a good way to practice some input/output, variables, looping, switch statements, etc. Anyway, I'm sure that I probably butchered the code and did a terrible job at it, so I was wondering if you all could maybe tell me how I could have written it better?

#include <iostream>
#include <conio.h>
#include <stdlib.h>
#include <time.h>
using namespace std;

int main()
{
                 //srand(time(NULL));

    int pHP = 100;
    int pATK = 5;
    int pLVL = 1;

    int mHP = 100;
    int mATK = 5;
    int mLVL = 1;

    int random;
    int atkDmg;
    int mRandom;
    int mAtkDmg;

    cout << "Welcome to the battle simulator! You will be battling a unarmed bandit today! Go you!" << endl;

    //Game Loop
    while(mHP > 0 && pHP > 0)
    {
        int choice;
        cout << "What would you like to do?" << endl
             << "1 - Attack" << endl;

        cin >> choice;

        switch(choice)
        {
            case 1:
                srand(time(NULL));
                random = rand() % 5 + 1;
                atkDmg = (pATK * random);
                mHP = mHP - (atkDmg);
                cout << "You hit bandit for " << atkDmg << "." << endl;
                    if (mHP <= 0)
                    {
                        //battleWin(pLVL, mLVL);
                        cout << "You have killed the bandit!" << endl;
                        break;
                    }
                cout << mHP << endl;
                break;

            default:
                break;
        };

        //Bandits turn, will always attack
        if(mHP > 0)
        {
            mRandom = rand() % 5 + 1;
            mAtkDmg = (mATK * mRandom);
            pHP = pHP - (mAtkDmg);
            cout << "You have been hit by the bandit for " << mAtkDmg << "!" << endl;
            cout << pHP << endl;
            if (pHP <= 0)
            {
                cout << "You have been killed by the bandit!" << endl;
            }
        }


    }
return 0;
}

[PredicateNormative]

Here are some suggestions:

1) Always initialise variables with a value.
2) I'm interested why you commented out correctly calling srand once at the beginning in favour of incorrectly calling srand multiple times. You should only seed the random number generator once, else you'll not get the randomness you expect.
3) Remove the C includes <conio.h>, <stdlib.h> and <time.h>, and instead use the mode C++ friendly includes <cstdlib> and <ctime>.
4) Naming convention: many people would expect a prefix of p on a variable name to mean that it is a pointer, but that is a matter of style.
5) Start to think about object orientation, else you will not really gain much in the way of development understanding over using basic C programming. For example, both your player and the bandit require hit points, attack damage and a level. Therefore you could wrap this up into a class. The advantage of using classes is that you can then store many characters in a container such as an std::list or an std::vector. You could also expand the functionality of the class to include other things such as a character description, weapons, medikits and such like. e.g.

class Character
{
public:
  Character(const std::string& name, const std::string& description = "a human", int hitPoints = 100, int attackPoints = 100, int level = 1)
    :m_name(name) //This is called an initialisation list
    ,m_description(description)
    ,m_hitPoints(hitPoints)
    ,m_attackPoints(attackPoints)
    ,m_level(level)
  {}

  const std::string& name()         const { return m_name;           }
  const std::string& description()  const { return m_description;    }
  int                hitPoints()    const { return m_hitPoints;      }
  int                attackPoints() const { return m_attackPoints;   }
  int                level()        const { return m_level;          }
  bool               isDead()       const { return m_hitPoints <= 0; }

  void hitPoints(int value)
  {
    m_hitPoints = value;
  }

  void attackPoints(int value)
  {
    m_attackPoints = value;
  }

  void level(int value)
  {
    m_attackPoints = value;
  }

private:
  std::string m_name;
  std::string m_description;
  int         m_hitPoints;
  int         m_attackPoints;
  int         m_level;
};

To get you into object orientation, I would suggest either using, or writing something like the above class to manage your characters and storing them into a class that you will need to write called CharacterManager. This will keep a list of characters for your game. You could then create a class called BattleManager to manage the battles in your game.

If you get stuck, I'm sure someone on the forum will help you (I'm rarely here any more).

[KruSuPhy]

Thanks for your criticism, but I have some answers to your questions:

1. I tried initializing my variables with a value, but I believe when I tried doing that, I kept encountering the "crosses initialization of" error, and to work around that, I just created the variables beforehand, and then set a value to them later.
2. I called srand twice because when i only did it once, both the player and the bandit were hitting each other for the same amount of damage each time. i tested it a couple times and it happened every time. Therefore, my workaround for that was to call it multiple times to get a different random value for each atkDMG.
3. I don't really have an excuse for this one, so thanks, and I will do that!
4. Same as #3, but thanks for the advice!
5. My excuse for this one is that I planned on doing this later on. I plan on working on this project as I learn until it's a full text RPG with shops, battling, NPCs and other stuff like that. I know it'll be expansive for a text RPG, but I think it's a good learning experience to get into game development, which is my main reasoning for learning C++. I actually started working with SFML before I fully understood the basics of C++, which is why I'm coming back to do that before I actually try and make anything graphical.

[Paul McKenzie]

5. My excuse for this one is that I planned on doing this later on. I plan on working on this project as I learn until it's a full text RPG with shops, battling, NPCs and other stuff like that.

You're not really learning anything sustantial if you just write more functions and declare more variables. You'll have a 10,000 line, procedural program that has basically the same programming sophistication as a 100 line procedural program that could have been written in any language, even non-OO languages such as C, Fortran 77 or Algol. So are you really learning C++ in terms of mindset and thought or just using initialization, for-loops. and function calls? That is what D_Drmmr's point was.

The hurdle is to think not of just "functional decomposition", but of object design. Yes, at the "low-level" you need to write functions, but you have to get into the mindset of creating objects. Don't be discouraged, as even experienced, non-OO programmers can have a difficult time thinking in the "object paradigm", and instead jump immediately into writing functions.

Regards,

Paul McKenzie

[KruSuPhy]

const std::string& name()         const { return m_name;           }

Can someone help me with this tidbit? I understand that you call the return type, which is std::string, name it(name), and the { return m_name; } part. However, why is there a const right before { return m_name;}? What function does that serve? and Also, I've read about pointers and stuff a decent amount, because I don't understand them very well, but what's the use of std::string& name()? What functionality does doing that give over just calling it std::string name()?

[Paul McKenzie]

const std::string& name()         const { return m_name;           }

Can someone help me with this tidbit? I understand that you call the return type, which is std::string, name it(name), and the { return m_name; } part. However, why is there a const right before { return m_name;}? What function does that serve? and Also, I've read about pointers and stuff a decent amount, because I don't understand them very well, but what's the use of std::string& name()? What functionality does doing that give over just calling it std::string name()?

http://www.parashift.com/c++-faq/overview-const.html

Regards,

Paul McKenzie

[KruSuPhy]

Thanks for the link, I'm looking into it right now. From that page, I've gotten into looking at what Pass by reference is.
So what I'm understanding now is that const std::string& must be the same thing as std::string const&, which would be pass-by-referencing name() in my code, and that the const that comes up after the function declaration means that the function cannot modify name() at all. Am I correct in this, or completely wrong?

[PredicateNormative]

Thanks for the link, I'm looking into it right now. From that page, I've gotten into looking at what Pass by reference is.
So what I'm understanding now is that const std::string& must be the same thing as std::string const&, which would be pass-by-referencing name() in my code,

I'm not entirely sure what you mean when you say "which would be pass-by-referencing name() in my code". However, for clarification,

const std::string& name() const { return m_name; }

is saying that there is a member function called name() that returns a const reference to m_name.

and that the const that comes up after the function declaration means that the function cannot modify name() at all

Close, it means that name() has a promise not to modify the internals of the Character instance on which it is acting.

[KruSuPhy]

I meant that you would be passing name() by reference as opposed to by value.

Anyway, I tried to take the advice and went and attempted to re-make the game from a more Object-Oriented mindset, using the Character class(mostly) that Predicate wrote. I took that and added a Battle class, which handles the battle between a bandit and a traveler. I'm not sure how I'd make a CharacterManager, but I'll also try to do that later. I'm not even sure how well I did it today.

In this code you'll notice that I commented out a section, and that's because I was trying to write something that would decide who went first by using even/odd numbers. However, I went first every single time, so I just commented that part out. You'll also notice that though the attack damage is different every round, both the traveler and bandit have the same attack damage per round. So you both would die at the same time every time, if the bandit could attack after his hitPoints hit 0. However, he can't, so you win every time and I'm not sure how to fix that other than calling srand twice.

#include <iostream>
#include <cstdlib>
#include <ctime>

class Character
{
public:
    Character(const std::string name, int hitPoints = 100, int attackPoints = 5, int level = 1)
        : m_name(name)
        , m_hitPoints(hitPoints)
        , m_attackPoints(attackPoints)
        , m_level(level)
    {}

    const std::string& name()           const {return m_name;}
    int                hitPoints()      const {return m_hitPoints;}
    int                attackPoints()   const {return m_attackPoints;}
    int                level()          const {return m_level;}
    bool               isDead()         const {return m_hitPoints <= 0;}

    void hitPoints(int value)
    {
        m_hitPoints = value;
    }

    void attackPoints(int value)
    {
        m_attackPoints = value;
    }

    void level(int value)
    {
        m_level = value;
    }

private:

    std::string m_name;
    int         m_hitPoints;
    int         m_attackPoints;
    int         m_level;
};

class Battle
{
public:
    Battle(Character &player, Character &npc)
    {
        isPlayersTurn = true;
        battleStart(player, npc);

    }





private:

    //int randomHundred = rand() % 100 + 1;
    //int randomAttack = rand() % 5 + 1;
    int attackDamage(Character &attacker);
    bool isPlayersTurn;
    void battleStart(Character &player, Character &npc);






};

int Battle::attackDamage(Character &attacker)
{
    srand(time(NULL));
    int random = rand() % 5 + 1;
    int trueDamage;
    trueDamage = attacker.attackPoints() * random;
    return trueDamage;
}

void Battle::battleStart(Character &player, Character &npc)
{
    srand(time(NULL));
    std::cout << npc.name() << " has attacked " << player.name() << "!" << std::endl;

//    if ((randomHundred%2)==0)
//    {
//        isPlayersTurn = true;
//        std::cout << "You will go first!" << std::endl;
//
//    }else{
//        isPlayersTurn = false;
//        std::cout << npc.name() << " will go first!" << std::endl;
//    }

    while (player.hitPoints() > 0 && npc.hitPoints() > 0)
    {
        if (isPlayersTurn)
        {
            loop:
            std::cout << "What would you like to do?" << std::endl << "1 - Attack" << std::endl;
            std::cout << player.hitPoints() << " " << npc.hitPoints() << std::endl;
            int choice;
            std::cin >> choice;

            switch(choice)
            {
            case 1:
                {
                int damage = attackDamage(player);
                npc.hitPoints(npc.hitPoints() - damage);
                std::cout << "You hit the enemy for " << damage << "!" << std::endl;
                std::cout << player.hitPoints() << " " << npc.hitPoints() << std::endl;
                isPlayersTurn = false;
                //deathCheck(player, npc);
                break;
                }

            default:
                goto loop;
                break;


            }

        }else{
            int damage = attackDamage(npc);
            player.hitPoints(player.hitPoints() - damage);
            std::cout << "You were hit by the enemy for " << damage << "!" << std::endl;
            std::cout << player.hitPoints() << " " << npc.hitPoints() << std::endl << std::endl;

            isPlayersTurn = true;
            //goto loop;

            //deathCheck;
        }


    }

    if (player.hitPoints() > 0 && npc.hitPoints() <= 0)
    {
        std::cout << "Congratulations! You have beaten " << npc.name() << "!" << std::endl;
        player.level(player.level() + 1);
        std::cout << "You have raised in level to " << player.level() << "!" << std::endl;

    }else if (player.hitPoints() <= 0 && npc.hitPoints() > 0){
        std::cout << "You have been killed by " << npc.name() << "!" << std::endl;
        std::cout << player.hitPoints() << " " << npc.hitPoints() << std::endl;
    }

}

int main()
{
    Character traveler("Traveler John", 100, 5, 1);
    Character bandit("Bandit Michael", 100, 5, 1);

    Battle wildBattle(traveler, bandit);


    std::cout << traveler.level() << std::endl;


}

I also might have went a little overboard with the &player and &name. I don't know if I even used it right.

[D_Drmmr]

Anyway, I tried to take the advice and went and attempted to re-make the game from a more Object-Oriented mindset, using the Character class(mostly) that Predicate wrote. I took that and added a Battle class, which handles the battle between a bandit and a traveler. I'm not sure how I'd make a CharacterManager, but I'll also try to do that later. I'm not even sure how well I did it today.

An "OO mindset" doesn't simply mean writing classes. You should design your program to use classes effectively. One way in which your original program can benefit from classes, is to organize your code/data. You have similar variables for player and bandit. By moving these variables into a class and creating two instances of that class, you make it easier to manage the code. E.g. if you'd want to add an armor or defense rating, you only need to do it in one place, instead of two. Also if you need to add more types of characters, you need to add less code.

Another benefit of using classes is that you can encapsulate variables, meaning you control access (particularly write access) to certain variables. E.g. instead of having setter functions that allow the hitPoints, attackPoints or level of a Character to be set from anywhere in your program, you can write functions that alter these variables in accordance with certain logic. Right now, you are settings the hitPoints from various places. You are not even using the isDead() function in Character, but instead duplicate the logic that that function is supposed to encapsulate. By duplicating code, you make it hard to maintain code, because when something changes (or you find a bug) you will have to change code in a lot of places... and it's hard to know if you have covered all those places that need a change or not.

Your Battle class should not be a class at all. All this really is, is a function. After you create an instance of Battle, there is nothing you can do with that instance. That should be an indication to you that the class should be removed.

[KruSuPhy]

Thanks for the tips. I've gotten rid of the battle class, and just changed it to a function. I have a question for a 'bug' that I'm not quite sure how to fix.
During battles, the player and npc will hit each other for a random amount, but they hit each other for the same random amount each turn. i.e, they'll both hit for 15, then 20, then 10, etc.
I'm pretty sure it's because time is only measured down to the second, and I call the random damage more than once a second. you can see in my code up there, since i haven't really changed anything. can someone help?

[2kaud]

You only use srand() once, usually at the beginning of main(). You don't use it before every call to rand().

[KruSuPhy]

I'm not sure if you were saying that just as a tip, or if you meant it was the fix to my problem, but I removed the extra srand()s and the player and NPC are now hitting for different amounts each time, so thank you!