11
\$\begingroup\$

I'm new to coding and this code pretty much goes to the extent of my current knowledge I'm looking for ways to improve this code. This code compiles and runs fine in Visual Studio 2013. I'm also using Windows 8.1 as my current OS for the computer I'm writing and compiling on.

I'd like a review over anything and any errors with the math involved in the game or the errors with the game.

What this code is suppose to do and from my testing does. Is be a simple game in which there are two players taking pegs off a ring one player is the user (human), the other player is a computer. Depending on the what moves the user makes is the computer responds with an ideal move. If there is no ideal move then the computer makes a random move of taking 1 ring off a random peg basically attempting to slow the game down giving more chances for the human user to make a mistake.

The math involved in this basically makes it so player 1 can always win. The computer is to know every ideal move and react based on human user moves. But since the computer can go second he is then just waiting for a error made by the human user or trying to slow down the game.

#include<iostream>
#include<string>
#include<stdlib.h>
#include<time.h>

using namespace std;

int main()
{
    char cAgain;
    do
    {
        //Variable Declartion
        int x, PegTotal, PegOne = 3, PegTwo = 5, PegThree = 7, Player, y;
        bool switchRepeat = true;
        cout << "Welcome to the 3 Peg game!!!" << endl;
        cout << "This is a 2 player game." << endl << endl;
        cout << "The game has 3 pegs and 15 rings." << endl
            << "The first peg has 3 rings, the second peg has 5, and the third peg has 7." << endl
            << "The object of the game is to get the other player to take off the last ring." << endl << endl;
        system("PAUSE");
        system("CLS");
        cout << "The rules of the game are as follow:" << endl
            << "1. You must take at least one ring off a peg per turn." << endl
            << "2. You can only take rings off a single peg in a given turn." << endl
            << "3. You can take as many rings off a single peg as you like." << endl
            << "4. You cannot skip a turn." << endl
            << "5. The player who takes the last ring off loses." << endl << endl;
        system("PAUSE");
        system("CLS");

        do
        {
            cout << "Would you like to go first or second?(Type 1 or 2)" << endl;
            cin >> Player;


            if (Player == 1)
            {
                cout << "You are player one!" << endl
                    << "You will go first." << endl << endl;
                goto check;
            }
            else if (Player == 2)
            {
                cout << "You are player 2!" << endl
                    << "You will go second" << endl << endl;
                cout << "Game Master takes 3 rings off peg 3" << endl;
                PegThree = 4;
                do
                {
                    check:
                    system("PAUSE");
                    system("CLS");
                    do
                    {
                        PegTotal = PegOne + PegTwo + PegThree;
                        if (PegTotal == 1)
                        {
                            cout << "Peg One has " << PegOne << " rings left." << endl
                                << "Peg Two has " << PegTwo << " rings left." << endl
                                << "Peg Three has " << PegThree << " rings left." << endl << endl;
                            cout << "You must take the last ring. Your Lose!!!" << endl;
                            system("PAUSE");
                            system("CLS");
                            goto end;
                        }
                        else if (PegTotal == 0)
                        {
                            break;
                        }
                        else
                            cout << "Peg One has " << PegOne << " rings left." << endl
                            << "Peg Two has " << PegTwo << " rings left." << endl
                            << "Peg Three has " << PegThree << " rings left." << endl << endl;
                        cout << "Which peg will you take rings from?" << endl;
                        cin >> x;
                        if ((x == 1) && (PegOne == 0) || (x == 2) && (PegTwo == 0) || (x == 3) && (PegThree == 0))
                        {
                            cout << "That peg is empty." << endl;
                            system("PAUSE");
                            system("CLS");
                        }
                        else if (x != 1 && x != 2 && x != 3)
                        {
                            cout << "That is not a peg." << endl;
                            system("PAUSE");
                            system("CLS");
                        }
                    } while ((x != 1 && x != 2 && x != 3) || ((x == 1) && (PegOne == 0) || (x == 2) && (PegTwo == 0) || (x == 3) && (PegThree == 0)));


                    do
                    {
                        switch (x)
                        {
                        case 1:
                            cout << "How many rings would you like to take from peg " << x << ". Max(" << PegOne << ")." << endl;
                            cin >> y;
                            if (y > PegOne)
                            {
                                cout << "There are not that many rings on that peg." << endl << endl;
                                system("PAUSE");
                                system("CLS");
                                break;
                            }
                            else
                            {
                                PegOne = PegOne - y;
                                y = 0;
                                switchRepeat = false;
                                break;
                            }
                        case 2:
                            cout << "How many rings would you like to take from peg " << x << ". Max(" << PegTwo << ")." << endl;
                            cin >> y;
                            if (y > PegTwo)
                            {
                                cout << "There are not that many rings on that peg." << endl << endl;
                                system("PAUSE");
                                system("CLS");
                                break;
                            }
                            else
                            {
                                PegTwo = PegTwo - y;
                                y = 0;
                                switchRepeat = false;
                                break;
                            }
                        case 3:
                            cout << "How many rings would you like to take from peg " << x << ". Max(" << PegThree << ")." << endl;
                            cin >> y;
                            if (y > PegThree)
                            {
                                cout << "There are not that many rings on that peg." << endl << endl;
                                system("PAUSE");
                                system("CLS");
                                break;
                            }
                            else
                            {
                                PegThree = PegThree - y;
                                y = 0;
                                switchRepeat = false;
                                break;
                            }
                        default:
                            switchRepeat = false;
                            break;
                        }
                    } while (switchRepeat == true);
                    PegTotal = PegOne + PegTwo + PegThree;

                    //----------------------------Peg Three Operations----------------------------
                    if ((PegThree > PegOne && PegThree> PegTwo) && PegThree > 1)
                    {
                        if (PegThree > PegOne && PegTwo > PegOne && PegOne != 0)
                        {
                            PegThree = PegThree - PegOne;
                            cout << "The Game Master takes " << PegOne << " rings from peg three." << endl;
                            //cout << "3-1" << endl;
                        }
                        else if ((PegTwo == 0 && PegOne == 1) || (PegOne == 0 && PegTwo == 1))
                        {
                            cout << "The Game Master takes " << PegThree << " rings from peg three." << endl;
                            PegThree = 0;
                            //cout << "3-2" << endl;
                        }
                        else if (PegThree > PegTwo && PegOne > PegTwo && PegTwo != 0)
                        {
                            PegThree = PegThree - PegTwo;
                            cout << "The Game Master takes " << PegTwo << " rings from peg three." << endl;
                            //cout << "3-3" << endl;
                        }
                        else if (PegTwo == 0 && PegOne == 0)
                        {
                            cout << "The Game Master takes " << (PegThree-1) << " rings from peg three." << endl;
                            PegThree = PegThree - (PegThree - 1);
                            //cout << "3-4" << endl;
                        }
                        else if ((PegThree > 3) > PegTwo >= (PegOne == 0) || ((PegThree > 3) > PegOne > (PegTwo == 0)))
                        {
                            PegThree--;
                            cout << "The Game Master takes 1 rings from peg three." << endl;
                            //cout << "3-5" << endl;
                        }
                    }
                    //-------------------------Peg Two Operations-------------------------------
                    else if ((PegTwo > PegOne && PegTwo > PegThree) && PegTwo > 1)
                    {
                        if (PegTwo > PegOne && PegThree > PegOne && PegOne != 0)
                        {
                            PegTwo = PegTwo - PegOne;
                            cout << "The Game Master takes " << PegOne << " rings from peg two." << endl;
                            //cout << "2-1" << endl;
                        }
                        else if ((PegThree == 0 && PegOne == 1) || (PegOne == 0 && PegThree == 1))
                        {
                            cout << "The Game Master takes " << PegTwo << " rings from peg three." << endl;
                            PegTwo = 0;
                            //cout << "2-2" << endl;
                        }
                        else if (PegTwo > PegThree && PegOne > PegThree && PegThree != 0)
                        {
                            PegTwo = PegTwo - PegThree;
                            cout << "The Game Master takes " << PegThree << " rings from peg two." << endl;
                            //cout << "2-3" << endl;
                        }
                        else if (PegThree == 0 && PegOne == 0)
                        {
                            cout << "The Game Master takes " << (PegTwo-1) << " rings from peg two." << endl;
                            PegTwo = PegTwo - (PegTwo - 1);
                            //cout << "2-4" << endl;
                        }
                        else if ((PegTwo > 3) > PegThree >= (PegOne == 0) || ((PegTwo > 3) > PegOne > (PegThree == 0)))
                        {
                            PegTwo--;
                            cout << "The Game Master takes 1 rings from peg two." << endl;
                            //cout << "2-5" << endl;
                        }
                    }
                    //----------------------Peg One Operations---------------------------------
                    else if ((PegOne > PegTwo && PegOne >> PegThree) && PegOne > 1)
                    {
                        if (PegOne > PegTwo && PegTwo > PegThree && PegThree != 0)
                        {
                            PegOne = PegOne - PegThree;
                            cout << "The Game Master takes " << PegThree << " rings from peg one." << endl;
                            //cout << "1-1" << endl;
                        }
                        else if ((PegTwo == 0 && PegThree == 1) || (PegThree == 0 && PegTwo == 1))
                        {
                            cout << "The Game Master takes " << PegOne << " rings from peg three." << endl;
                            PegOne = 0;
                            //cout << "1-2" << endl;
                        }
                        else if (PegOne > PegThree && PegThree > PegTwo && PegTwo != 0)
                        {
                            PegOne = PegOne - PegTwo;
                            cout << "The Game Master takes " << PegTwo << " rings from peg one." << endl;
                            //cout << "1-3" << endl;
                        }
                        else if (PegTwo == 0 && PegThree == 0)
                        {
                            cout << "The Game Master takes " << PegOne << " rings from peg one." << endl;
                            PegOne = PegOne - (PegOne - 1);
                            //cout << "1-4" << endl;
                        }
                        else if ((PegOne > 3) > PegTwo >= (PegThree == 0) || ((PegOne > 3) > PegThree > (PegTwo == 0)))
                        {
                            PegOne--;
                            cout << "The Game Master takes 1 rings from peg one." << endl;
                            //cout << "1-5" << endl;
                        }
                    }
                    //------------------Computer must take last ring------------------
                    else if (PegTotal == 1)
                    {
                        cout << "The Game Master takes the last ring." << endl
                            << "You Win!!!" << endl;
                        goto end;
                    }
                    //-------------------Computer has no winning scenarios-------------
                    else
                    {
                        do
                        {
                            srand(time(NULL));
                            x = rand() % 3 + 1;
                            if ((x == 1) && (PegOne == 0) || (x == 2) && (PegTwo == 0) || (x == 3) && (PegThree == 0))
                            {
                                //cout << "Error 01 x= " << x << endl;
                            }
                            else if (x != 1 && x != 2 && x != 3)
                            {
                                //cout << "Error 02 x= "<< x << endl;
                            }
                            else
                            {
                                cout << "The Game Master takes 1 ring from peg " << x << "." << endl;
                                break;
                            }
                        } while ((x != 1 && x != 2 && x != 3) || ((x == 1) && (PegOne == 0) || (x == 2) && (PegTwo == 0) || (x == 3) && (PegThree == 0)));

                        if ((x == 1 && PegOne != 0))
                        {
                            PegOne = PegOne - 1;
                            //cout << "PegOne-1" << endl;
                            goto check;
                        }
                        else if ((x == 2 && PegTwo != 0))
                        {
                            PegTwo = PegTwo - 1;
                            //cout << "PegTwo-1" << endl;
                            goto check;
                        }
                        else if ((x == 3 && PegThree != 0))
                        {
                            PegThree = PegThree - 1;
                            //cout << "PegThree-1" << endl;
                            goto check;
                        }
                    }
                } while (PegOne > 0 || PegTwo > 0 || PegThree > 0);
            }
            else
            {
                cout << "You must choose to be player 1 or 2." << endl;
                system("PAUSE");
                system("CLS");
            }
        } while (Player != 1 && Player != 2);
        end:
        cout << "Would you like to play again?(Y/N)" << endl;
            cin >> cAgain;
            system("CLS");
    } while (cAgain == 'Y' || cAgain == 'y');
return 0;
}

Here is the .cpp file.

\$\endgroup\$
18
\$\begingroup\$
using namespace std;

This can be a bad habit to develop. See Why is using namespace std bad practice?

    //Variable Declartion

Typo.

    //Variable Declarations
    int x, PegTotal, PegOne = 3, PegTwo = 5, PegThree = 7, Player, y;
    bool switchRepeat = true;

In C++, you don't have to declare your variables at the start. You can declare them just before you start using them. This helps with keeping track of what each variable is.

In this case, you probably should declare some of those in a class. However, that's a pretty big leap from the current state of the code.

Try to capitalize consistently. The general standards for a variable name are either camelCase or snake_case. The switchRepeat variable is camelCase, so you should probably make Player and PegTotal into player and pegTotal respectively.

If you see yourself giving variables names with numbers in them, consider using a collection to hold them instead.

    std::array<int, 3> pegs = { 3, 5, 7 };

I make collections plural. Now pegs[0] is equal to 3, just as PegOne was.

    system("PAUSE");
    system("CLS");

Using system commands like this can be risky. See Edward's answer here for more information.

You have an incredible amount of nesting and should use functions to abstract out some of the functionality. E.g.

void showHelp() {
    cout << "Welcome to the 3 Peg game!!!" << endl;
    cout << "This is a 2 player game." << endl << endl;
    cout << "The game has 3 pegs and 15 rings." << endl
         << "The first peg has 3 rings, the second peg has 5, and the third peg has 7." << endl
         << "The object of the game is to get the other player to take off the last ring." << endl << endl;
    system("PAUSE");
    system("CLS");
    cout << "The rules of the game are as follow:" << endl
         << "1. You must take at least one ring off a peg per turn." << endl
         << "2. You can only take rings off a single peg in a given turn." << endl
         << "3. You can take as many rings off a single peg as you like." << endl
         << "4. You cannot skip a turn." << endl
         << "5. The player who takes the last ring off loses." << endl << endl;
    system("PAUSE");
    system("CLS");
}

Now you can just call showHelp() whenever you want to display this.

        cout << "Would you like to go first or second?(Type 1 or 2)" << endl;
        cin >> Player;


        if (Player == 1)
        {
            cout << "You are player one!" << endl
                << "You will go first." << endl << endl;
            goto check;
        }
        else if (Player == 2)
        {
            cout << "You are player 2!" << endl
                << "You will go second" << endl << endl;
            cout << "Game Master takes 3 rings off peg 3" << endl;
            PegThree = 4;
            do
            {
                check:

In this case, it's trivial to write the code such that a goto isn't necessary. Note that you don't have to nest the do with check: in it inside the else clause.

    do
    {
        cout << "Would you like to go first or second?(Type 1 or 2)" << endl;
        cin >> player;
    } while ( player != 1 && player != 2 );


    if (player == 1)
    {
        cout << "You are player one!" << endl
             << "You will go first." << endl << endl;
    }
    else 
    {
        cout << "You are player 2!" << endl
             << "You will go second" << endl << endl;
        cout << "Game Master takes 3 rings off peg 3" << endl;
        pegs[2] = 4;
    }

    do
    {
        check:

This way, you get rid of two layers of nesting as well as eliminating a goto. But we probably want to go ahead and make a couple functions out of this too.

enum Turn { YOU, GAME_MASTER };

But first, let's declare an enum. With this, we can specify who is doing what without using integers.

void removeRings(std::array<int,3> & pegs, std::size_t peg, int quantity, Turn turn)
{
    switch ( turn )
    {
        case YOU:
            std::cout << "You take ";
            break;
        case GAME_MASTER:
            std::cout << "Game Master takes ";
            break;
    }

    std::cout << quantity << " rings off peg "
    std::cout << peg + 1 << endl;

    pegs[peg] -= quantity;
}

This function just provides a standard way to represent a move. By defining a function, we put all instances where we're announcing a move in the same place. In your original code, if you wanted to change the way that this was output, you'd have to make changes all over the place.

We introduce the idea of function parameters. The first parameter, pegs, is passed by reference. You can tell because it has a & between the type and the variable name that will be used in the function. We give pegs the same name in the function as it had in the caller, but we did not have to do so. It was simply the best name in both places. Passed by reference means that we don't have to make a new copy of the object and allows us to modify the original object while in the function. The other three parameters are passed by value.

At the end, pegs[peg] -= quantity; is the same as writing pegs[peg] = pegs[peg] - quantity;. It just saves repeating pegs[peg].

You can see it used in the next function:

void chooseTurn(std::array<int,3> & pegs)
{
    int player = 0;

    while ( true )
    {
        cout << "Would you like to go first or second?(Type 1 or 2)" << endl;
        cin >> player;

        if ( 1 == player )
        {
            cout << "You are player one!" << endl
                 << "You will go first." << endl << endl;
            return;
        }
        else if ( 2 == player )
        {
            cout << "You are player 2!" << endl
                 << "You will go second" << endl << endl;
            removeRings(pegs, 2, 3, GAME_MASTER);
            return;
        }
        else
        {
            cout << "You must choose to be player 1 or 2." << endl;
            system("PAUSE");
            system("CLS");
        }
    }
}

Now we have this code in a function. There's an argument that this should be two functions in that it reads an input value and it responds, but I'm not a stickler for purity. Doing it this way means that we never need player outside this function.

We also move the else clause much closer to the other clauses. I missed it entirely when first writing this function. I only found it because I was checking where player is defined.

I flipped around the order of the equality check: 1 == player. The reason to do this is that in most C style languages, if ( player = 1 ) is a valid statement, but 1 = player is not. The latter version will give a compiler error if you make that typo. The first version will quietly assign 1 to player and evaluate as true for the if.

Note: I would usually prefer not to use the loop forever method. The reason why it's needed here is to avoid repeating the same checks as the if.

        PegTotal = PegOne + PegTwo + PegThree;

Since we have pegs now, we can write this more generally.

        pegTotal = std::accumulate(pegs.begin(), pegs.end(), 0);

Now even if we add more pegs, the code will total all of them.

        if (PegTotal == 1)
        {
            cout << "Peg One has " << PegOne << " rings left." << endl
                << "Peg Two has " << PegTwo << " rings left." << endl
                << "Peg Three has " << PegThree << " rings left." << endl << endl;
            cout << "You must take the last ring. Your Lose!!!" << endl;
            system("PAUSE");
            system("CLS");
            goto end;
        }
        else if (PegTotal == 0)
        {
            break;
        }
        else
            cout << "Peg One has " << PegOne << " rings left." << endl
            << "Peg Two has " << PegTwo << " rings left." << endl
            << "Peg Three has " << PegThree << " rings left." << endl << endl;

This can be written more simply:

        if ( 0 == PegTotal )
        {
            break;
        }

        displayPegs(pegs);

        if ( 1 == PegTotal )
        {
            cout << "You must take the last ring. You Lose!!!" << endl;
            system("PAUSE");
            system("CLS");
            goto end;
        }

First, check the 0 case. Since it has a break, it doesn't need an else.

Second, after that, we display the peg counts in both tracks. So display it first. Use a function to abstract it.

Third, we don't need an else for the 1 case either. We already displayed the peg counts, which is all the else did.

Finally, move it before the loop where we get the player's peg choice. Until the player makes a choice, this won't change.

void displayPegs(const std::array<int,3> & pegs)
{
    cout << "Peg One has " << pegs[0] << " rings left." << endl
         << "Peg Two has " << pegs[1] << " rings left." << endl
         << "Peg Three has " << pegs[2] << " rings left." << endl << endl;
}

We pass pegs as a const reference, short for constant reference. This just means that we do not change pegs in this function. We only read from it.

                do
                {
                    cout << "Which peg will you take rings from?" << endl;
                    cin >> x;
                    if ((x == 1) && (PegOne == 0) || (x == 2) && (PegTwo == 0) || (x == 3) && (PegThree == 0))
                    {
                        cout << "That peg is empty." << endl;
                        system("PAUSE");
                        system("CLS");
                    }
                    else if (x != 1 && x != 2 && x != 3)
                    {
                        cout << "That is not a peg." << endl;
                        system("PAUSE");
                        system("CLS");
                    }
                } while ((x != 1 && x != 2 && x != 3) || ((x == 1) && (PegOne == 0) || (x == 2) && (PegTwo == 0) || (x == 3) && (PegThree == 0)));

We can move all this into a function:

                x = choosePeg(pegs);

Which we define as follows:

std::size_t choosePeg(const std::array<int,3> & pegs)
{
    std::size_t x;

    while ( true )
    {
        cout << "Which peg will you take rings from?" << endl;
        cin >> x;

        if ( 1 > x || pegs.size() < x )
        {
            cout << "That is not a peg." << endl;
            system("PAUSE");
            system("CLS");
        }
        else if ( 0 >= pegs(--x) )
        {
            cout << "That peg is empty." << endl;
            system("PAUSE");
            system("CLS");
        }
        else
        {
            return x;
        }

        displayPegs(pegs);
    }
}

We loop forever, getting a peg choice from standard input. We check if the choice is in the collection's range. If not, we tell the user that is not a peg. If it is, we check if there are any rings on that peg. If not, we warn the user. If yes, we return that value of x to the caller (which ends the loop). If we don't return, we display the pegs again and restart the loop.

I changed the equality check to an inequality. I realize that they should never be less than zero, but this costs us nothing and will catch even if there is a bug that allows them to be less than zero.

                        if (y > PegOne)
                        {
                            cout << "There are not that many rings on that peg." << endl << endl;
                            system("PAUSE");
                            system("CLS");
                            break;
                        }
                        else
                        {
                            PegOne = PegOne - y;
                            y = 0;
                            switchRepeat = false;
                            break;
                        }

You don't need to write break; in both branches of the if here.

                        if (y > PegOne)
                        {
                            cout << "There are not that many rings on that peg." << endl << endl;
                            system("PAUSE");
                            system("CLS");
                        }
                        else
                        {
                            PegOne = PegOne - y;
                            y = 0;
                            switchRepeat = false;
                        }

                        break;

As a general rule, try to avoid repeating code if you don't have to do so.

In this case though, we can avoid the switch statement entirely. The different cases can be simplified down to one and put into a function.

void removeRings(std::array<int,3> & pegs, std::size_t peg)
{
    std::size_t display_peg = peg + 1;

    while ( true )
    {
        int ring_count = 0;

        cout << "How many rings would you like to take from peg " << display_peg << ". Max(" << pegs(peg) << ")." << endl;
        cin >> ring_count;

        if ( ring_count > pegs[peg] )
        {
            cout << "There are not that many rings on that peg." << endl << endl;
            system("PAUSE");
            system("CLS");
        }
        else
        {
            removeRings(pegs, peg, ring_count, YOU);
            return;
        }
    }
}

We did something interesting here. We overloaded a function by redefining it with different parameters. Now the compiler will determine which function to call by the parameters in the call.

I renamed x and y to better describe what they are.

I added a new variable display_peg, since I'm using a 0-based index for the array but the user is still working with natural numbers.

                        PegThree = PegThree - PegOne;
                        cout << "The Game Master takes " << PegOne << " rings from peg three." << endl;

We replace each occurrence like that with a call to removeRings.

                        removeRings(pegs, 2, pegs[0], GAME_MASTER);

Remember to adjust because our array index is off by one from our display.

                //------------------Computer must take last ring------------------
                else if (PegTotal == 1)

This is a simple check, so you should do it before the other checks.

                        srand(time(NULL));

You should do this before starting play, outside all the loops. Note that it is possible to call this multiple times within a single second. Each time you do this, you reseed the random counter with the same value. This actually makes the result repeat.

                        x = rand() % 3 + 1;

Since we are using the container, you can replace this with

                        x = rand() % pegs.size();

Now it will scale automatically if more pegs are added.

                        if ((x == 1) && (PegOne == 0) || (x == 2) && (PegTwo == 0) || (x == 3) && (PegThree == 0))

This can be written more simply now:

                        if ( 0 == pegs[x] )

Again, this will scale if more pegs are added.

                        else if (x != 1 && x != 2 && x != 3)

This wasn't needed and still isn't. x will always be in the right range, as that's how you set up the randomization.

                            cout << "The Game Master takes 1 ring from peg " << x << "." << endl;

This can be a removeRings call.

                            removeRings(pegs, x, 1, GAME_MASTER);

This also eliminates the next section, which removes the ring from the appropriate peg.

return 0;

You don't have to do this in main. The compiler will do it for you if you leave it off.

This review is already long, so I won't try to cover more. If you submit a new question as an iterative review, you could get more suggestions. In particular, I'd actually prefer to do this as a class so as not to pass pegs to every function. Someone else might have critiques of some of my suggestions.

#include <iostream>
#include <string>
#include <stdlib.h>
#include <time.h>
#include <array>
#include <numeric>

void showHelp()
{
    std::cout << "Welcome to the 3 Peg game!!!" << std::endl;
    std::cout << "This is a 2 player game." << std::endl << std::endl;
    std::cout << "The game has 3 pegs and 15 rings." << std::endl
              << "The first peg has 3 rings, the second peg has 5, and the third peg has 7." << std::endl
              << "The object of the game is to get the other player to take off the last ring." << std::endl << std::endl;
    system("PAUSE");
    system("CLS");
    std::cout << "The rules of the game are as follow:" << std::endl
              << "1. You must take at least one ring off a peg per turn." << std::endl
              << "2. You can only take rings off a single peg in a given turn." << std::endl
              << "3. You can take as many rings off a single peg as you like." << std::endl
              << "4. You cannot skip a turn." << std::endl
              << "5. The player who takes the last ring off loses." << std::endl << std::endl;
    system("PAUSE");
    system("CLS");
}

enum Turn { YOU, GAME_MASTER };

void removeRings(std::array<int,3> & pegs, std::size_t peg, int quantity, Turn turn)
{
    switch ( turn )
    {
        case YOU:
            std::cout << "You take ";
            break;
        case GAME_MASTER:
            std::cout << "Game Master takes ";
            break;
    }

    std::cout << quantity << " rings off peg ";
    std::cout << peg + 1 << std::endl;

    pegs[peg] -= quantity;
}

void removeRings(std::array<int,3> & pegs, std::size_t peg)
{
    std::size_t display_peg = peg + 1;

    while ( true )
    {
        int ring_count = 0;

        std::cout << "How many rings would you like to take from peg " << display_peg << ". Max(" << pegs[peg] << ")." << std::endl;
        std::cin >> ring_count;

        if ( ring_count > pegs[peg] )
        {
            std::cout << "There are not that many rings on that peg." << std::endl << std::endl;
            system("PAUSE");
            system("CLS");
        }
        else
        {
            removeRings(pegs, peg, ring_count, YOU);
            return;
        }
    }
}

void chooseTurn(std::array<int,3> & pegs)
{
    int player = 0;

    while ( true )
    {
        std::cout << "Would you like to go first or second?(Type 1 or 2)" << std::endl;
        std::cin >> player;

        if ( 1 == player )
        {
            std::cout << "You are player one!" << std::endl
                      << "You will go first." << std::endl << std::endl;
            return;
        }
        else if ( 2 == player )
        {
            std::cout << "You are player 2!" << std::endl
                      << "You will go second" << std::endl << std::endl;
            removeRings(pegs, 2, 3, GAME_MASTER);
            return;
        }
        else
        {
            std::cout << "You must choose to be player 1 or 2." << std::endl;
            system("PAUSE");
            system("CLS");
        }
    }
}

void displayPegs(const std::array<int,3> & pegs)
{
    std::cout << "Peg One has " << pegs[0] << " rings left." << std::endl
              << "Peg Two has " << pegs[1] << " rings left." << std::endl
              << "Peg Three has " << pegs[2] << " rings left." << std::endl << std::endl;
}

std::size_t choosePeg(const std::array<int,3> & pegs)
{
    std::size_t x;

    while ( true )
    {
        std::cout << "Which peg will you take rings from?" << std::endl;
        std::cin >> x;

        if ( 1 > x || pegs.size() < x )
        {
            std::cout << "That is not a peg." << std::endl;
            system("PAUSE");
            system("CLS");
        }
        else if ( 0 >= pegs[--x] )
        {
            std::cout << "That peg is empty." << std::endl;
            system("PAUSE");
            system("CLS");
        }
        else
        {
            return x;
        }

        displayPegs(pegs);
    }
}

int main()
{
    char cAgain;

    srand(time(NULL));

    do
    {
        //Variable Declarations
        std::array<int,3> pegs = { 3, 5, 7 };

        showHelp();
        chooseTurn(pegs);
        int pegTotal = std::accumulate(pegs.begin(), pegs.end(), 0);

        do
        {
            system("PAUSE");
            system("CLS");

            if ( 0 == pegTotal )
            {
                break;
            }

            displayPegs(pegs);

            if ( 1 == pegTotal )
            {
                std::cout << "You must take the last ring. You Lose!!!" << std::endl;
                system("PAUSE");
                system("CLS");
                break;
            }

            int x = choosePeg(pegs);
            removeRings(pegs, x);

            pegTotal = std::accumulate(pegs.begin(), pegs.end(), 0);

            if (pegTotal == 1)
            {
            //------------------Computer must take last ring------------------
                std::cout << "The Game Master takes the last ring." << std::endl
                          << "You Win!!!" << std::endl;
                break;
            }
            else if ( pegs[2] > pegs[0] &&  pegs[2] >  pegs[1] && pegs[2] > 1)
            {
            //----------------------------Peg Three Operations----------------------------
                if ( pegs[1] >  pegs[0] &&  pegs[0] != 0)
                {
                     removeRings(pegs, 2, pegs[0], GAME_MASTER);
                }
                else if ( pegs[1] + pegs[0] == 1 )
                {
                    removeRings(pegs, 2, pegs[2], GAME_MASTER);
                }
                else if ( pegs[0] >  pegs[1] &&  pegs[1] != 0)
                {
                    removeRings(pegs, 2, pegs[1], GAME_MASTER);
                }
                else if ( pegs[1] == 0 &&  pegs[0] == 0)
                {
                    removeRings(pegs, 2, pegs[2] - 1, GAME_MASTER);
                }
                else if (( pegs[2] > 3) >  pegs[1] >= ( pegs[0] == 0) || (( pegs[2] > 3) >  pegs[0] > ( pegs[1] == 0)))
                {
                    removeRings(pegs, 2, 1, GAME_MASTER);
                }
            }
            else if (( pegs[1] >  pegs[0] &&  pegs[1] >  pegs[2]) &&  pegs[1] > 1)
            {
            //-------------------------Peg Two Operations-------------------------------
                if ( pegs[2] >  pegs[0] &&  pegs[0] > 0)
                {
                    removeRings(pegs, 1, pegs[0], GAME_MASTER);
                }
                else if ( pegs[2] + pegs[0] == 1 )
                {
                    removeRings(pegs, 1, pegs[1], GAME_MASTER);
                }
                else if ( pegs[0] >  pegs[2] &&  pegs[2] > 0)
                {
                    removeRings(pegs, 1, pegs[2], GAME_MASTER);
                }
                else if ( pegs[2] == 0 &&  pegs[0] == 0)
                {
                    removeRings(pegs, 1, pegs[1] - 1, GAME_MASTER);
                }
                else if (( pegs[1] > 3) >  pegs[2] >= ( pegs[0] == 0) || (( pegs[1] > 3) >  pegs[0] > ( pegs[2] == 0)))
                {
                    removeRings(pegs, 1, 1, GAME_MASTER);
                }
            }
            else if (( pegs[0] >  pegs[1] &&  pegs[0] > pegs[2]) &&  pegs[0] > 1)
            {
            //----------------------Peg One Operations---------------------------------
                if ( pegs[1] >  pegs[2] &&  pegs[2] > 0)
                {
                    removeRings(pegs, 0, 2, GAME_MASTER);
                }
                else if ( pegs[1] + pegs[2] == 1 )
                {
                    removeRings(pegs, 0, pegs[0], GAME_MASTER);
                }
                else if ( pegs[2] >  pegs[1] && pegs[1] > 0 )
                {
                    removeRings(pegs, 0, pegs[1], GAME_MASTER);
                }
                else if ( pegs[1] == 0 &&  pegs[2] == 0)
                {
                    removeRings(pegs, 0, pegs[0] - 1, GAME_MASTER);
                }
                else if (( pegs[0] > 3) >  pegs[1] >= ( pegs[2] == 0) || (( pegs[0] > 3) >  pegs[2] > ( pegs[1] == 0)))
                {
                    removeRings(pegs, 0, 1, GAME_MASTER);
                }
            }
            else
            {
            //-------------------Computer has no winning scenarios-------------
                while ( true )
                {
                    x = rand() % 3;
                    if ( 0 < pegs[x] )
                    {
                        removeRings(pegs, x, 1, GAME_MASTER);
                        break;
                    }
                }

            }
            pegTotal = std::accumulate(pegs.begin(), pegs.end(), 0);
        } while ( pegTotal > 0);

        std::cout << "Would you like to play again?(Y/N)" << std::endl;
        std::cin >> cAgain;
        system("CLS");
    } while (cAgain == 'Y' || cAgain == 'y');
}

I verified that this compiles, but I'm not set up to run it.

\$\endgroup\$
  • \$\begingroup\$ in removeRings() what if the user inputs a negative number? That should be controlled too. \$\endgroup\$ – dabadaba Dec 24 '14 at 22:55
  • \$\begingroup\$ @Brythan this review was great thanks for all the help. I haven't gotten to fully read through it all, but one question I do have is why not use using std::(cout, cin, endl, etc) instead of typing out each time. \$\endgroup\$ – Styx Dec 25 '14 at 0:35
  • \$\begingroup\$ @Styx That's covered in the Why is using namespace std bad practice? link from the beginning of the review. \$\endgroup\$ – Brythan Dec 25 '14 at 0:59
  • \$\begingroup\$ A nit-pick suggestion: replace system("PAUSE"); with std::cin.sync(); std::cin.ignore(); or something, then a simple %s/CLS/clear/g in vim, and this code compiles & runs on linux just as easily \$\endgroup\$ – Elias Van Ootegem Jan 22 '15 at 11:10
  • 1
    \$\begingroup\$ @EliasVanOotegem Why not put that in an answer? That's code from the original question, so there's no reason you couldn't make that an answer. Then you could explain more about how your solution works and how it differs from the original. I'm reluctant to try to incorporate it here, as I'm not set up to actually run the code. \$\endgroup\$ – Brythan Jan 22 '15 at 11:25
4
\$\begingroup\$

As I commented below @Brythan's review (which I agree with, BTW), I'd avoid the system calls you're currently using. The PAUSE and CLS calls are, of course, platform specific, so while your code does compile on a *NIX system, the program won't behave as expected.
For that reason, I'd suggest you change system("PAUSE") to something along the lines of:

std::cin.ignore(80, '\n');
std::cin.sync();

The first statement reads either 80 chars from the input buffer (or less, if it encounters a \n first), effectively causing your program to wait for user input. The input is, as the method name suggests, ignored (as in discarded) however.
It could be that the input buffer contains far more than 80 chars, though, which is why std::cin.sync(); is being called. This call is to clear the input buffer or whatever input there might be left in there, in case the user input looks something like:

The object of the game is to get the other player to take off the last ring.

some really long string of user input, well over 80 chars long. Unlikely, but possible. \n

By my count, the input here is 87 chars, so std::cin.ignore will leave the last 7 chars in the buffer ("ible. \n"), but the std::cin.sync call removes them.

As for the system("CLS"); call, that's a little trickier, seeing as C/C++ both lack IO capalities and therefore don't really know what a screen is. You could use a windows-specific header that contains a clrscr - which I believe is the conio header, and use ncurses for *NIX systems, use a make file that links/builds everything depending on what system you're compiling on, but that's a lot of hassle for a task as trivial as this.
There are some X-platform C++ frameworks that might solve this issue, too, like Qt (C++) or GTK (c), but again: I'm not going to stop you if you want to take the time and put in the effort to learn these libs/frameworks, but you're trying to clear the screen, a task for which there must be an easier fix.

The approach I'd suggest is to define a macro, based on the OS on which your code is compiled. According to MSDN, the predefined __WIN32 preprocessor macro should do cf the list of predefined macro's. I've tested it, and got the expected results by adding this:

#ifdef __WIN32
#define CLEAR_SCREEN "CLS"
#else
#define CLEAR_SCREEN "clear"
#endif

Then, I changed system("CLS"); to system(CLEAR_SCREEN) and that was the end of that. Just fore completeness' sake, the full code below (just copy-pasted from @Brythan's review, and applied the 2 changes I discussed here):

#include <iostream>
#include <string>
#include <stdlib.h>
#include <time.h>
#include <array>
#include <numeric>

#ifdef __WIN32
#define CLEAR_SCREEN "CLS"
#else
#define CLEAR_SCREEN "clear"
#endif

void showHelp()
{
    std::cout << "Welcome to the 3 Peg game!!!" << std::endl;
    std::cout << "This is a 2 player game." << std::endl << std::endl;
    std::cout << "The game has 3 pegs and 15 rings." << std::endl
              << "The first peg has 3 rings, the second peg has 5, and the third peg has 7." << std::endl
              << "The object of the game is to get the other player to take off the last ring." << std::endl << std::endl;
    std::cin.ignore(80, '\n');
    std::cin.sync();
    system(CLEAR_SCREEN);
    std::cout << "The rules of the game are as follow:" << std::endl
              << "1. You must take at least one ring off a peg per turn." << std::endl
              << "2. You can only take rings off a single peg in a given turn." << std::endl
              << "3. You can take as many rings off a single peg as you like." << std::endl
              << "4. You cannot skip a turn." << std::endl
              << "5. The player who takes the last ring off loses." << std::endl << std::endl;
    std::cin.ignore(80, '\n');
    std::cin.sync();
    system(CLEAR_SCREEN);
}

enum Turn { YOU, GAME_MASTER };

void removeRings(std::array<int,3> & pegs, std::size_t peg, int quantity, Turn turn)
{
    switch ( turn )
    {
        case YOU:
            std::cout << "You take ";
            break;
        case GAME_MASTER:
            std::cout << "Game Master takes ";
            break;
    }

    std::cout << quantity << " rings off peg ";
    std::cout << peg + 1 << std::endl;

    pegs[peg] -= quantity;
}

void removeRings(std::array<int,3> & pegs, std::size_t peg)
{
    std::size_t display_peg = peg + 1;

    while ( true )
    {
        int ring_count = 0;

        std::cout << "How many rings would you like to take from peg " << display_peg << ". Max(" << pegs[peg] << ")." << std::endl;
        std::cin >> ring_count;

        if ( ring_count > pegs[peg] )
        {
            std::cout << "There are not that many rings on that peg." << std::endl << std::endl;
            std::cin.ignore(80, '\n');
            std::cin.sync();
            system(CLEAR_SCREEN);
        }
        else
        {
            removeRings(pegs, peg, ring_count, YOU);
            return;
        }
    }
}

void chooseTurn(std::array<int,3> & pegs)
{
    int player = 0;

    while ( true )
    {
        std::cout << "Would you like to go first or second?(Type 1 or 2)" << std::endl;
        std::cin >> player;

        if ( 1 == player )
        {
            std::cout << "You are player one!" << std::endl
                      << "You will go first." << std::endl << std::endl;
            return;
        }
        else if ( 2 == player )
        {
            std::cout << "You are player 2!" << std::endl
                      << "You will go second" << std::endl << std::endl;
            removeRings(pegs, 2, 3, GAME_MASTER);
            return;
        }
        else
        {
            std::cout << "You must choose to be player 1 or 2." << std::endl;
            std::cin.ignore(80, '\n');
            std::cin.sync();
            system(CLEAR_SCREEN);
        }
    }
}

void displayPegs(const std::array<int,3> & pegs)
{
    std::cout << "Peg One has " << pegs[0] << " rings left." << std::endl
              << "Peg Two has " << pegs[1] << " rings left." << std::endl
              << "Peg Three has " << pegs[2] << " rings left." << std::endl << std::endl;
}

std::size_t choosePeg(const std::array<int,3> & pegs)
{
    std::size_t x;

    while ( true )
    {
        std::cout << "Which peg will you take rings from?" << std::endl;
        std::cin >> x;

        if ( 1 > x || pegs.size() < x )
        {
            std::cout << "That is not a peg." << std::endl;
            std::cin.ignore(80, '\n');
            std::cin.sync();
            system(CLEAR_SCREEN);
        }
        else if ( 0 >= pegs[--x] )
        {
            std::cout << "That peg is empty." << std::endl;
            std::cin.ignore(80, '\n');
            std::cin.sync();
            system(CLEAR_SCREEN);
        }
        else
        {
            return x;
        }

        displayPegs(pegs);
    }
}

int main()
{
    char cAgain;

    srand(time(NULL));

    do
    {
        //Variable Declarations
        std::array<int,3> pegs = { 3, 5, 7 };

        showHelp();
        chooseTurn(pegs);
        int pegTotal = std::accumulate(pegs.begin(), pegs.end(), 0);

        do
        {
            std::cin.ignore(80, '\n');
            std::cin.sync();
            system(CLEAR_SCREEN);

            if ( 0 == pegTotal )
            {
                break;
            }

            displayPegs(pegs);

            if ( 1 == pegTotal )
            {
                std::cout << "You must take the last ring. You Lose!!!" << std::endl;
                std::cin.ignore(80, '\n');
                std::cin.sync();
                system(CLEAR_SCREEN);
                break;
            }

            int x = choosePeg(pegs);
            removeRings(pegs, x);

            pegTotal = std::accumulate(pegs.begin(), pegs.end(), 0);

            if (pegTotal == 1)
            {
            //------------------Computer must take last ring------------------
                std::cout << "The Game Master takes the last ring." << std::endl
                          << "You Win!!!" << std::endl;
                break;
            }
            else if ( pegs[2] > pegs[0] &&  pegs[2] >  pegs[1] && pegs[2] > 1)
            {
            //----------------------------Peg Three Operations----------------------------
                if ( pegs[1] >  pegs[0] &&  pegs[0] != 0)
                {
                     removeRings(pegs, 2, pegs[0], GAME_MASTER);
                }
                else if ( pegs[1] + pegs[0] == 1 )
                {
                    removeRings(pegs, 2, pegs[2], GAME_MASTER);
                }
                else if ( pegs[0] >  pegs[1] &&  pegs[1] != 0)
                {
                    removeRings(pegs, 2, pegs[1], GAME_MASTER);
                }
                else if ( pegs[1] == 0 &&  pegs[0] == 0)
                {
                    removeRings(pegs, 2, pegs[2] - 1, GAME_MASTER);
                }
                else if (( pegs[2] > 3) >  pegs[1] >= ( pegs[0] == 0) || (( pegs[2] > 3) >  pegs[0] > ( pegs[1] == 0)))
                {
                    removeRings(pegs, 2, 1, GAME_MASTER);
                }
            }
            else if (( pegs[1] >  pegs[0] &&  pegs[1] >  pegs[2]) &&  pegs[1] > 1)
            {
            //-------------------------Peg Two Operations-------------------------------
                if ( pegs[2] >  pegs[0] &&  pegs[0] > 0)
                {
                    removeRings(pegs, 1, pegs[0], GAME_MASTER);
                }
                else if ( pegs[2] + pegs[0] == 1 )
                {
                    removeRings(pegs, 1, pegs[1], GAME_MASTER);
                }
                else if ( pegs[0] >  pegs[2] &&  pegs[2] > 0)
                {
                    removeRings(pegs, 1, pegs[2], GAME_MASTER);
                }
                else if ( pegs[2] == 0 &&  pegs[0] == 0)
                {
                    removeRings(pegs, 1, pegs[1] - 1, GAME_MASTER);
                }
                else if (( pegs[1] > 3) >  pegs[2] >= ( pegs[0] == 0) || (( pegs[1] > 3) >  pegs[0] > ( pegs[2] == 0)))
                {
                    removeRings(pegs, 1, 1, GAME_MASTER);
                }
            }
            else if (( pegs[0] >  pegs[1] &&  pegs[0] > pegs[2]) &&  pegs[0] > 1)
            {
            //----------------------Peg One Operations---------------------------------
                if ( pegs[1] >  pegs[2] &&  pegs[2] > 0)
                {
                    removeRings(pegs, 0, 2, GAME_MASTER);
                }
                else if ( pegs[1] + pegs[2] == 1 )
                {
                    removeRings(pegs, 0, pegs[0], GAME_MASTER);
                }
                else if ( pegs[2] >  pegs[1] && pegs[1] > 0 )
                {
                    removeRings(pegs, 0, pegs[1], GAME_MASTER);
                }
                else if ( pegs[1] == 0 &&  pegs[2] == 0)
                {
                    removeRings(pegs, 0, pegs[0] - 1, GAME_MASTER);
                }
                else if (( pegs[0] > 3) >  pegs[1] >= ( pegs[2] == 0) || (( pegs[0] > 3) >  pegs[2] > ( pegs[1] == 0)))
                {
                    removeRings(pegs, 0, 1, GAME_MASTER);
                }
            }
            else
            {
            //-------------------Computer has no winning scenarios-------------
                while ( true )
                {
                    x = rand() % 3;
                    if ( 0 < pegs[x] )
                    {
                        removeRings(pegs, x, 1, GAME_MASTER);
                        break;
                    }
                }

            }
            pegTotal = std::accumulate(pegs.begin(), pegs.end(), 0);
        } while ( pegTotal > 0);

        std::cout << "Would you like to play again?(Y/N)" << std::endl;
        std::cin >> cAgain;
        system(CLEAR_SCREEN);
    } while (cAgain == 'Y' || cAgain == 'y');
}

It compiled just fine on linux using this command:

g++ -std=c11 peg.cpp -o peg

I did see some warnings pop up when I added some optimization, and told the compiler to warn me about possible issues:

g++ -std=c11 peg.cpp -o peg -O3 -Wall

In case you want to know, a breakdown of the command:

  • g++ is the compiler
  • -std=c++11 is the C++ standard used to compile the code (C++11 being true to the C++11 standard, gnu++11 being the same + GNU extensions, so no longer pure standard C++).
  • peg.cpp refers to the source file (obviously)
  • -o peg is the compiled binary.
  • -O3 is the optimization level (details here)
  • -Wall is just asking the compiler to warn me about all potential issues.

The issues the compiler reported were:

  • Ignoring the return value of system(CLEAR_SCREEN); (very low priority to fix)
  • Possible nonsensical comparisons (Readability is an issue, I'd fix it)

The comparison warnings included:

peg.cpp:268:94: warning: comparisons like ‘X<=Y<=Z’ do not have their mathematical meaning [-Wparentheses]
else if (( pegs[0] > 3) > pegs[1] >= ( pegs[2] == 0) || (( pegs[0] > 3) > pegs[2] > ( pegs[1] == 0)))

Which is indeed not the easiest code to read, perhaps consider refactoring the if-elseif conditions to a format that is easier to read/maintain:

else if (( pegs[1] > 3) >  pegs[2] >= ( pegs[0] == 0) || (( pegs[1] > 3) >  pegs[0] > ( pegs[2] == 0)))

At first glance, it's basically the same as writing:

else if ( pegs[1] > 3 && ((!pegs[0] && !pegs[2]) || (!pegs[2] && pegs[0])))

But, AFAIK, you could've just written else if (pegs[1] > 3), or just else... either way, those conditions can be simplified a lot

Be that as it may, the code (after my minor changes) ran like clockwork. Starting the game was as easy as doing:

./peg
\$\endgroup\$

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.