3
\$\begingroup\$

I'm a few months into learning C++ programming and I want to know if I'm moving generally in the right direction or not with the following code. This is the most advanced thing I've created so far, but it doesn't contain any pointers or references so I'm worried I'm not doing things properly on the memory level. I especially want advice regarding moving this type of code to templates and virtual functions.

//This program's purpose is a simple battle simulator and a simple lotto simulator
//This requires two classes and a bit of procedural main game logic that can repeat

//The first class is a Randomizer class that allows the user to generate
//random numbers by inputting the desired amount of numbers, and the max
//range for those numbers. The default start value is 1.

//The second class is a Battler class that contains two Randomizer objects.
//It also contains a method to compare the second integer of each of the vector
//arrays contained in each of those objects, and returns -1, 0, or 1 depending
//on which value is greater, or 0 if they are tied.

#include <iostream>     //Necessary for cout and endl
#include <ctime>        //Necessary for time_t variable
#include <cstdlib>      //Necessary for rand and srand
#include <vector>       //Necessary for vector<type>

/*HEADER OF RANDOMIZER CLASS*/
class Randomizer    //The preferred syntax is to start with a capital letter for classes
{
    private:
    time_t timevalue;           //time_t member variable that will be set later
    void randomTimeInit();      //called by constructor (runs once)

    public:
    Randomizer()    //Class constructor
    {
    srand(time(NULL));      //This is JUST necessary here to get new results every time
                            //Caution: Only run once per program execution! (per object?)
    void randomTimeInit();  //This function sets time_t value and seeds rand() with it
    std::cout << "construct randomizer" << std::endl;   //UNCOMMENT FOR DEBUG
    }
    ~Randomizer()   //Class deconstructor
    {
    std::cout << "destruct randomizer" << std::endl;    //UNCOMMENT FOR DEBUG
    }

    std::vector<int> randomIntArray;        //The vector array for the random numbers
    int getRandomNumber(int);               //Function to return one random number of max int
    void createRandomIntArray(int, int);    //Function to create random number array
    void printRandomIntArray();             //Function to print created random number array
};


/*IMPLEMENTATION OF RANDOMIZER CLASS*/
void Randomizer::randomTimeInit()
{
    time_t timevalue;   //time_t is a special value (usually number of seconds since 00:00)
    time(&timevalue);   //Sets a reference timevalue to the time (requires the special time_t value)
    srand(timevalue);   //Seeds the random generator with timevalue
}

int Randomizer::getRandomNumber(int max)
{
    return rand() % max +1;    //Returns random int %modulo (max value) + 1 (to eliminate 0)
}

void Randomizer::createRandomIntArray(int count, int max)
{
    for(int i=1;i<=count;i++)  //Iterates up to the count value
    {
        int randomInt = getRandomNumber(max);                       //Sets randomInt to 1-to-max
        randomIntArray.insert(randomIntArray.begin(), randomInt);   //Inserts randomInt at start of vector
    }
}

void Randomizer::printRandomIntArray()
{
    for(unsigned int j=0;j<randomIntArray.size();j++)        //Iterates up to previously created vector size
    {
        std::cout << randomIntArray[j] << " ";               //Outputs number value at [j] followed by a space
    }
}


/*HEADER OF BATTLER CLASS*/
class Battler
{
    private:
    Randomizer random1, random2;            //Two Randomizer objects that will each have different values
    int numOne, maxOne, numTwo, maxTwo;     //Four variables for setting up the battle

    public:
    Battler()   //Class constructor
    {
        std::cout << "con battler" << std::endl;    //UNCOMMENT FOR DEBUG
    }
    ~Battler()  //Class destructor
    {
        std::cout << "decon battler" << std::endl;  //UNCOMMENT FOR DEBUG
    }

    int doBattler();        //Function that compares values between the Randomizer objects
};


/*IMPLEMENTATION OF BATTLER CLASS*/
int Battler::doBattler()
{
    numOne = 5;     //Five numbers to choose from for the battle (arbitrary)
    maxOne = 100;   //Value up to 100 for the "damage" number (arbitrary)
    numTwo = 5;     //Five numbers to choose from for the battle (arbitrary)
    maxTwo = 100;   //Value up to 100 for the "damage" number (arbitrary)
    random1.createRandomIntArray(numOne, maxOne);   //Generates Player 1s numbers
    random2.createRandomIntArray(numTwo, maxTwo);   //Generates Player 2s numbers
    std::cout << "Here the two scores:" << std::endl;
    random1.printRandomIntArray();      //Prints Player 1s numbers
    std::cout << std::endl;
    random2.printRandomIntArray();      //Prints Player 2s numbers
    std::cout << std::endl;

    //This begins the logic to test which player won the round
    //I decided to use a simple return here instead of a switch statement
    //One definite downside of the following code is having to repeat two of the lines
    //in each of the three different statements in order to clear the vector array between rounds
    if(random1.randomIntArray[2] == random2.randomIntArray[2])  //NOTE: this looks at the second integer
                                                                //of the member arrays and compares
                                                                //them ((arbitrary) but it has to be a value
                                                                //inside the array or it crashes)
    {
        std::cout <<"It was a draw!" << std::endl << std::endl;
        random1.randomIntArray.clear();     //This is necessary to clear the vector array between rounds
        random2.randomIntArray.clear();     //If it's not present then the array continues to expand forever
        return 0;                           //Return immediately stops the function and returns this value
    }
    else if(random1.randomIntArray[2] > random2.randomIntArray[2])  //See NOTE above
    {
        std::cout << "Player 1 hits!" << std::endl << std::endl;
        random1.randomIntArray.clear();     //This is necessary to clear the vector array between rounds
        random2.randomIntArray.clear();     //If it's not present then the array continues to expand forever
        return -1;                          //Return immediately stops the function and returns this value
    }
    else
    {
        std::cout << "Player 2 hits!" << std::endl << std::endl;
        random1.randomIntArray.clear();     //This is necessary to clear the vector array between rounds
        random2.randomIntArray.clear();     //If it's not present then the array continues to expand forever
        return 1;                           //Return immediately stops the function and returns this value
    }
}


/*START OF FUTURE MAIN.CPP*/
/*PROTOTYPE DECLARATIONS*/
void inputAndPrintRandomIntArray();  //This function takes inputs for amount of numbers  and
                                     //their max value and prints out the created array
void lottoLoop();                    //This function allows the repetition of the above function
                                     //if the user inputs char 'Y'
void battleLoop();                   //This function creates a Battler object and player life
                                     //variables and repeats the doBattler function until one player is dead
void chooseBattleOrLotto();          //This function allows user input to control which of the two
                                     //main functions of the program to run

/*START OF MAIN FUNCTION*/
int main(int argc, char* args[])     //These arguments for main allow command line access?
{
    chooseBattleOrLotto();           //Allows user to choose which function program will perform
    return 0;                        //Here because int main expects a return value
}


/*DEFINITION OF MAIN.CPP FUNCTIONS*/
void inputAndPrintRandomIntArray()
{
    Randomizer myRandomizer;    //Instantiates an object of the Randomizer class
    int number, maximum;        //These are the variables that will go into Randomizer method
    std::cout << "Enter the amount of numbers for the Lotto Ticket (1-100):";
    std::cin >> number;
    std::cout << "Enter the maximum number from which they will be chosen(1-1000):";
    std::cin >> maximum;
    std::cout << "Here are your Lotto Numbers:" << std::endl;
    myRandomizer.createRandomIntArray(number, maximum);   //This activates Randomizer method with input values
    myRandomizer.printRandomIntArray();                   //This prints the created random int array
    std::cout << std::endl;
}

void lottoLoop()
{
    char gameRunning = 'Y';     //This char value will control whether the game continues to loop
    do
    {
        //system("cls");        //apparently dangerous to use, simply clears console
                                //alternative is cout << string( 100, '\n' ). Remove to see all outputs
        inputAndPrintRandomIntArray();  //Function that takes input and outputs vector array
        std::cout << "Would you like to play again? (must enter Y):";
        std::cin >> gameRunning;
    }
    while(gameRunning == 'Y');  //For some reason I couldn't get || 'y' to work
}

void battleLoop()
{
    Battler doSimpleBattle;            //Instantiates an object of the Battler class
    char battleAgain = 'Y';            //Creates a char that controls whether to repeat the battle

    //Begins the battle logic do/while loops
    do
    {
        int player1HP = 10;     //Setting the variables here resets them between games
        int player2HP = 10;     //Placing them where the char is above will not work
        do
        {
            int battleResult;                   //Creates a variable to control the if statements
            battleResult = doSimpleBattle.doBattler(); //Sets the variable to the return value of the function
            if(battleResult == -1)              //Looks at the return of the doBattler method
            {
                player2HP--;                    //Decrements Player 2 health based on this result
            }
            if(battleResult == 1)               //Continues looking at return of doBattler method
            {
                player1HP--;                    //Decrements Player 1 health based on this result
            }
        }
        while(player1HP > 0  || player2HP > 0); //Continue the game until one player has no health left

        if(player1HP == 0)                      //Checks if Player 1's health is zero
        {
            std::cout << "Player 2 wins!" << std::endl;
        }
        else if(player2HP == 0)                 //Checks if Player 2's health is zero
        {
            std::cout << "Player 1 wins!" << std::endl;
        }
        std::cout << "Game over!" << std::endl;
        std::cout << "Play again? (Must be Y to work)";
        std::cin >> battleAgain;                //This is the char input to control repeating the game
        std::cout << std::endl;
    }
    while(battleAgain == 'Y');                  //For some reason I can't get || 'y' to work
}

void chooseBattleOrLotto()
{
    char modeSelect = 'L';        //Creates a char that controls mode choice and exit
    std::cout << "Press 'L' for the lottery or 'B' for battle! ('Q' to exit):";
    std::cin >> modeSelect;
    if(modeSelect == 'L')
    {
        lottoLoop();              //Contains a do while loop that allows game repeat
    }
    if(modeSelect == 'B')
    {
        battleLoop();       //Contains a do while loop that runs the battle game with
                            //Player 1 vs Player 2 each having 10 HP that allows repeat
    }
    if(modeSelect == 'Q')
    {
        modeSelect = 'Q';
    }
    else
    {
        std::cout << "Value must match L or B. Q to Exit." << std::endl;
        if(modeSelect != 'Q')
        {
            chooseBattleOrLotto();  //Repeats entire function unless input char equals 'Q'
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

Some thoughts:

  1. I'm not sure why you need a randomizer class. The random number stream is a global resource, so srand() needs to be done only once. It may be better to rename this as a Player class, since that's what it's usually used for. I can see a use for a RandomStream class for your damage and lotto numbers, something like:

    class RandomStream
    {
    public:
        RandomStream( int max ) : m_max(max) {}
        virtual ~RandomStream() = 0;
        int Next() { return rand() % m_max + 1; }
    private:
        int m_max;
    };
    struct DamageGenerator: public RandomStream
    {
        DamageGenerator(): RandomStream(c_MaxDamage) {}
    };
    struct LottoGenerator: public RandomStream
    {
        LottoGenerator( int max ): RandomStream(max) {}
    };
    

    So your numbers can be generated as:

    LottoGenerator gen( maximum );
    for ( int i = 0; i < number; i++ ) {
    
        std::cout << gen.Next() << " ";
    }
    std::cout << std::endl;
    

    and your battle damage as:

    DamageGenerator m_gen;
    void GenerateScores( int num ) {
    
        m_scores.clear();
        for ( int i = 0; i < num; i++ ) {
    
            m_scores.push_back( m_gen.Next() );
        }
    }
    
  2. For writing out "constructor/destructor messages", try guarding them with a #ifdef

    #idef _DEBUG // or equivalent
    std::cout << "..." << std::endl;
    #endif
    

    That way, you don't need to remove them in "proper use".

  3. You have a loop running from 1 to <= count in one function and 0 to < size() in the other. Make the first one 0 to < count as well - more consistent.

  4. for

    randomIntArray.insert(randomIntArray.begin(), randomInt);
    

    you could do

    randomIntArray.push_back( randomInt );
    

    for simplicity - the order isn't significant.

  5. For printing them out, you could use an iterator:

    for ( std::vector<int>::iterator it = randomIntArray.begin(); it != randomIntArray.end(); it ++ ) {
        std::cout << *it << " "
    }
    

    Note that you're printing out a space at the end, which doesn't matter in this case. See C++ infix iterator for something rather more complex!

  6. Note that in your Randomizer::createRandomIntArray, there's nothing to stop you first doing

    randomIntArray.clear();
    

    This will remove the need for the annoying clears after the hit or draw messages.

  7. Don't use magic numbers 50, 100 and -1, 0, 1 etc. You can use

    const int c_Rounds = 5;
    const int c_MaxDamage = 100;
    

    and

    enum BattlerResult { PLAYER1_WINS, DRAW, PLAYER2_WINS };
    

    This is to aid readability (and also ease of change), rather than wondering what it means in the future.

  8. numOne, maxOne etc are only used once, so there's no need to "remember" them in the class definition - move them from there into the function (and actually, they're covered by the definition in 7) anyway).

  9. Try to keep Class/variable names in the realm of what they mean rather than what they do/are. e.g., Randomizer is probably better named as Player, random1 as player1, Battler as BattlerRound, doBattler as GetBattlerResult etc.

    • I know they're really notes to yourself, but in general keep comments explaining how/why, rather than just echoing the code they're commenting.
  10. The "who wins" code is repeated, possibly giving rise to inconsistent behaviour if one part is changed. Separate out that decision into a separate function, something like:

    BattlerResult result = DecideWinner( player1, player2 );
    
  11. Similarly, the hit or draw messages can be separated out into a function as well:

    GiveResultMessage( result );
    

    Doing these will simplify the overall doBattler function, making it easier to read and understand what's going on.

  12. When you receive input from an unvalidated source, validate it. It doesn't make sense to have -5 lotto numbers. Also, consider allowing e.g. b as well as B as input.

  13. The lotto loop termination should be:

    while ( ( gameRunning == 'Y' ) || ( gameRunning == 'y' ) )
    

    or perhaps even

    while ( ::toupper( gameRunning ) == 'Y' )
    
  14. The battleLoop loop could be a for, rather than a do...while; and it may be better as a boolean value (again, separating out the "play again" message as another function):

    for ( bool battleAgain = true; battleAgain; battleAgain = QueryAnotherGame() )...
    
  15. You may wish to introduce a BattleGame class to contain a single game, so that battleLoop just consists of:

    void battleLoop()
    {
        for ( bool battleAgain = true; battleAgain; battleAgain = QueryAnotherGame() )
        {
            BattleGame game;
            BattleResult result = game.Play();
            game.GiveWinnerMessage( result );
            std::cout << "Game over!" << std::endl;
        }
    }
    
  16. chooseBattleOrLotto may be better as a switch (such as):

    while ( true )
    {
        // ask L or B
        switch ( ::toupper( modeSelect ) )
        {
            case 'Q': return;
            case 'L': playLotto; break;
            case 'B': playBattle; break;
            default: "only L/B message"; break;
        }
    }
    

    Note that your function is recusive - it probably won't matter for this, but a person could crash the game if they were determined by repeatedly pressing an invalid key.

    I think templates and virtual functions are a bit too complex for this at the moment...

\$\endgroup\$
6
\$\begingroup\$

I suggest the following:

  • Member variables should be private.Do you really need

    std::vector<int> randomIntArray
    

    to be public in Randomizer?

  • Be const correct. Declare variables to be const whenever possible. The ff.

    int getRandomNumber(int); 
    void createRandomIntArray(int, int); 
    

    can be rewritten to:

    int getRandomNumber(const int);
    void createRandomIntArray(const int, const int); 
    
  • In terms of OOP design, I think it would be better if you could create another class named BattleGame ( or any other name ) where you will put your game logic.

    class BattleGame 
    {
    public:
        void chooseBattleOrLotto();
    
    private:
        void inputAndPrintRandomIntArray();
        void lottoLoop(); 
        void battleLoop();
     } 
    

You can then call it using:

int main(int argc, char* args[]) 
{
    BattleGame b;
    b.chooseBattleOrLotto();
    return 0;
}

This encapsulates the methods that the caller does not need to know.

There are probably more things you can do to improve your program. I will post some more suggestions when I get more time.

\$\endgroup\$
  • \$\begingroup\$ FAQ about const correctness: parashift.com/c++-faq-lite/const-correctness.html \$\endgroup\$ – idbrii Jul 3 '12 at 7:11
  • \$\begingroup\$ The concept of const correctness is a new one for me. The C++ Faq was extremely helpful. Thanks a lot for the feedback! \$\endgroup\$ – bazola Jul 3 '12 at 14:37
6
\$\begingroup\$

A couple quick observations:

class Randomizer

You're seeding your random number generator several times. srand is a global resource. You could create a single Randomizer object that passed by reference into the constructor of all objects -- that way only one is ever created. Or you could use a singleton. (Beware: Over-use of singletons is a common trap. Don't pretend they're not global variables. Read more here.)

In order to do the above, Randomizer should have no state. It's create and print functions should take a std::vector<int>& (reference) that they fill in or print.

class Battler

It's better style to declare multiple variables on multiple lines. (Because it sometimes doesn't do what you expect: char * a,b; declares one pointer and one char -- not two pointers.)

You have a lot of unnecessary member variables. Try to use local variables where possible to limit the shared data in your code. (More shared data means more opportunities for values you didn't expect.)

Battler::doBattler

It's better style to return enum or const values instead of "magic numbers".

Often when you have a function called "doSomething", it doesn't have precisely defined functionality (so it's hard to name). It looks like you want this to getBattleResult (and then the calling code could do the prints). Then your Battler is your model and the calling code is the UI.

Design

In terms of your design, instead of having an array of random numbers, you could just call getRandomNumber. If you're trying to add complexity (so you have a chance to use vectors), then you could use a series of randoms and average the top two.

Also, the C++ FAQ may be helpful. (It's one of my favourite resource for C++. Especially, the const correctness entry.)

\$\endgroup\$
  • \$\begingroup\$ I appreciate the feedback. The C++ Faq is indeed an amazing resource. Const correctness seems important as well. \$\endgroup\$ – bazola Jul 3 '12 at 14:50
  • \$\begingroup\$ The C++FAQ is awful. The author is opinionated and biased. Also some of the examples are just plain wrong. \$\endgroup\$ – Martin York Jul 3 '12 at 22:27
  • \$\begingroup\$ See: stackoverflow.com/tags/c%2b%2b/info for better references. \$\endgroup\$ – Martin York Jul 3 '12 at 22:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.