4
\$\begingroup\$

I am doing a little in-between semester reading in my C++ book, and I came up with the wonderful idea to try to make Yahtzee in C++. So far the code generates 5 random numbers and then calls the function that I made to display a rendering of the die. I cannot figure out how to allow the users to hold the dice that they want to keep for scoring, and I haven't developed a scoring system yet. I have essentially figured out how to roll some dice.

I know somebody is going to get on me for using using namespace std;. But basically I am looking for some direction on what to do next for re-rolling and keeping score.

// dice.cpp :
//

#include "stdafx.h"
#include <iostream>
#include <cstdlib>
#include <ctime>

void diceRoll();
void dice1();
void dice2();
void dice3();
void dice4();
void dice5();
void dice6();
using namespace std;

int getNum();
int main()
{
srand(static_cast<int>(time(0)));
diceRoll();
cout<<"Thanks for rollin my dice"<<endl;
system("pause");
        return 0;
}

void diceRoll()
{
int roll1=0;
int roll2=0;
int roll3=0;
int roll4=0;
int roll5=0;
char selection='y';
bool re1=' ';
bool re2=' ';
bool re3=' ';
bool re4=' ';
bool re5=' ';
char choice1=' ';
char choice2=' ';
char choice3=' ';
char choice4=' ';
char choice5=' ';
do{
cout<<"roll dice"<<endl;
roll1=getNum();
roll2=getNum();
roll3=getNum();
roll4=getNum();
roll5=getNum();

    switch(roll1)
    {
    case 1:
        dice1();
    break;

    case 2:
        dice2();
    break;

    case 3:
        dice3();
    break;

    case 4:
        dice4();
    break;

    case 5:
        dice5();
    break;

    case 6:
        dice6();
    break;

    }//endSwitch

    switch(roll2)
    {
    case 1:
        dice1();
    break;

    case 2:
        dice2();
    break;

    case 3:
        dice3();
    break;

    case 4:
        dice4();
    break;

    case 5:
        dice5();
    break;

    case 6:
        dice6();
    break;

    }//endSwitch


    switch(roll3)
    {
    case 1:
        dice1();
    break;

    case 2:
        dice2();
    break;

    case 3:
        dice3();
    break;

    case 4:
        dice4();
    break;

    case 5:
        dice5();
    break;

    case 6:
        dice6();
    break;

    }//endSwitch

    switch(roll4)
    {
    case 1:
        dice1();
    break;

    case 2:
        dice2();
    break;

    case 3:
        dice3();
    break;

    case 4:
        dice4();
    break;

    case 5:
        dice5();
    break;

    case 6:
        dice6();
    break;

    }//endSwitch


    switch(roll5)
    {
    case 1:
        dice1();
    break;

    case 2:
        dice2();
    break;

    case 3:
        dice3();
    break;

    case 4:
        dice4();
    break;

    case 5:
        dice5();
    break;

    case 6:
        dice6();
    break;

    }//endSwitch
cout<<"would you like to re-roll die 1 (y/n)"<<endl;
cin>>choice1;
if(choice1=='y')
{
    re1=true;
}
    if(re1=true)
    {
    roll1=getNum();
    switch(roll1)
    {
    case 1:
        dice1();
    break;

    case 2:
        dice2();
    break;

    case 3:
        dice3();
    break;

    case 4:
        dice4();
    break;

    case 5:
        dice5();
    break;

    case 6:
        dice6();
    break;

    }//endSwitch
}//endif
    if(choice1=='n')
    {
        cout<<roll1;
    }


cout<<"would you like to re-roll die 2 (y/n)"<<endl;
cin>>choice2;
if(choice2=='y')
{
    re2=true;
}
    if(re2=true)
    {
    roll2=getNum();
    switch(roll2)
    {
    case 1:
        dice1();
    break;

    case 2:
        dice2();
    break;

    case 3:
        dice3();
    break;

    case 4:
        dice4();
    break;

    case 5:
        dice5();
    break;

    case 6:
        dice6();
    break;

    }//endSwitch
    }//endif

cout<<"would you like to re-roll die 3 (y/n)"<<endl;
cin>>choice3;
if(choice3=='y')
{
    re3=true;
}
    if(re3=true)
    {
    roll3=getNum();
    switch(roll3)
    {
    case 1:
        dice1();
    break;

    case 2:
        dice2();
    break;

    case 3:
        dice3();
    break;

    case 4:
        dice4();
    break;

    case 5:
        dice5();
    break;

    case 6:
        dice6();
    break;

    }//endSwitch
    }//endif

cout<<"would you like to re-roll die 4 (y/n)"<<endl;
cin>>choice4;
if(choice4=='y')
{
    re4=true;
}
    if(re4=true)
    {
    roll4=getNum();
    switch(roll4)
    {
    case 1:
        dice1();
    break;

    case 2:
        dice2();
    break;

    case 3:
        dice3();
    break;

    case 4:
        dice4();
    break;

    case 5:
        dice5();
    break;

    case 6:
        dice6();
    break;

    }//endSwitch
    }//endif

cout<<"would you like to re-roll die 5 (y/n)"<<endl;
cin>>choice5;
if(choice5=='y')
{
    re5=true;
}
    if(re5=true)
    {
    roll5=getNum();

    switch(roll5)
    {
    case 1:
        dice1();
    break;

    case 2:
        dice2();
    break;

    case 3:
        dice3();
    break;

    case 4:
        dice4();
    break;

    case 5:
        dice5();
    break;

    case 6:
        dice6();
    break;

    }//endSwitch
    }//endif

cin>>selection;
system("cls");
}while(selection=='y');

}
int getNum()
{
int randNumber=0;
randNumber = 1+ rand() % (6-1+1);
return randNumber;
}
void dice1()
{
for(int row=0;row<5;row++)          
{   
   for(int spacer=0;spacer<8;spacer++)      
    {

        if(row==0||row==4)
        {
            cout<<"*";
        }


        if(row==1||row==3)
        {
            if(spacer==0||spacer==7)
            {
                cout<<"*";
            }
            else
                cout<<" ";
        }
        if (row==2)
            {
            if(spacer==1||spacer==2||spacer==3||spacer==5||spacer==6)
               {
                 cout<<" ";
               }//endif
            if(spacer==4)
               {
                 cout<<"x";
               }//endif
            if(spacer==0||spacer==7)
                {
                cout<<"*";
                }
             }

     }//endfor

    cout<<endl;
}   
}
void dice2()
{
for(int row=0;row<5;row++)          
{   
   for(int spacer=0;spacer<8;spacer++)      
    {

        if(row==0||row==4)
        {
            cout<<"*";
        }



        if (row==1||row==3)
            {
            if(spacer==1||spacer==2||spacer==3||spacer==5||spacer==6)
               {
                 cout<<" ";
               }//endif
            if(spacer==4)
               {
                 cout<<"x";
               }//endif
            if(spacer==0||spacer==7)
                {
                cout<<"*";
                }
             }
          if(row==2)
          {
          if(spacer==0||spacer==7)
            {
              cout<<"*";
            }             
          else
            cout<<" ";
          }

     }//endfor

    cout<<endl;
}
}
void dice3()
{
for(int row=0;row<5;row++)          
{   
   for(int spacer=0;spacer<8;spacer++)      
    {

        if(row==0||row==4)
        {
            cout<<"*";
        }



        if (row==1||row==2||row==3)
            {
            if(spacer==1||spacer==2||spacer==3||spacer==5||spacer==6)
               {
                 cout<<" ";
               }//endif
            if(spacer==4)
               {
                 cout<<"x";
               }//endif
            if(spacer==0||spacer==7)
                {
                cout<<"*";
                }
             }

     }//endfor

    cout<<endl;
}

}
void dice4()
{
for(int row=0;row<5;row++)          
{   
   for(int spacer=0;spacer<8;spacer++)      
    {

        if(row==0||row==4)
        {
            cout<<"*";
        }

        if (row==2)
        {

if(spacer==1||spacer==2||spacer==3||spacer==4||spacer==5||spacer==6)
            {
            cout<<" ";
            }
            if(spacer==0||spacer==7)
            {
            cout<<"*";
            }
        }

        if (row==1||row==3)
            {
            if(spacer==1||spacer==3||spacer==4||spacer==6)
               {
                 cout<<" ";
               }//endif
            if(spacer==2||spacer==5)
               {
                 cout<<"x";
               }//endif
            if(spacer==0||spacer==7)
                {
                cout<<"*";
                }
            }//endif


   }//endfor

    cout<<endl;
}
}
void dice5()
{
for(int row=0;row<5;row++)          
{   
   for(int spacer=0;spacer<8;spacer++)      
    {

        if(row==0||row==4)
        {
            cout<<"*";
        }

        if (row==2)
            {
            if(spacer==1||spacer==2||spacer==3||spacer==5||spacer==6)
               {
                 cout<<" ";
               }//endif
            if(spacer==4)
               {
                 cout<<"x";
               }//endif
            if(spacer==0||spacer==7)
                {
                cout<<"*";
                }
             }


        if (row==1||row==3)
            {
            if(spacer==1||spacer==3||spacer==4||spacer==6)
               {
                 cout<<" ";
               }//endif
            if(spacer==2||spacer==5)
               {
                 cout<<"x";
               }//endif
            if(spacer==0||spacer==7)
                {
                cout<<"*";
                }
            }//endif


   }//endfor

    cout<<endl;
}
}
void dice6()
{
for(int row=0;row<5;row++)          
{   
   for(int spacer=0;spacer<8;spacer++)      
    {

        if (row==1||row==2||row==3)
            {
            if(spacer==1||spacer==3||spacer==4||spacer==6)
               {
                 cout<<" ";
               }//endif
            if(spacer==2||spacer==5)
               {
                 cout<<"x";
               }//endif
            if(spacer==0||spacer==7)
                {
                cout<<"*";
                }
            }//endif
        else
        {
         cout<<"*";
        }

   }//endfor

    cout<<endl;
}
 }
Improve this question
\$\endgroup\$
6
  • \$\begingroup\$ Your best bet is to copy the code into visual studio and try it out. \$\endgroup\$
    – John Snow
    Jan 8, 2014 at 18:32
  • 1
    \$\begingroup\$ "What to do next" is off-topic for Code Review. We give suggestions for improving existing code, not how to write new code. But I am still upvoting this because I really think that your code is in desperate need of improvement. \$\endgroup\$
    – Simon Forsberg
    Jan 8, 2014 at 19:35
  • 2
    \$\begingroup\$ I haven't fully studied this yet, but it seems to me that much of this can be condensed by creating a new Die structure, perhaps as a class. I can't quite determine the purpose of each dice function. \$\endgroup\$
    – Jamal
    Jan 8, 2014 at 20:01
  • \$\begingroup\$ Take a look at this implementation of a die I've done. \$\endgroup\$
    – Jamal
    Jan 8, 2014 at 20:06
  • \$\begingroup\$ @Jamal it looks like the dice functions exist to display an ascii art representation of a die to cout. calling dice1() will display a die showing the face with 1 pip, calling dice6() will alternatively display a die showing 6 pips. \$\endgroup\$
    – YoungJohn
    Jan 8, 2014 at 20:42

3 Answers 3

Reset to default
4
\$\begingroup\$

Here's how you can improve your code:

  • Declaring function prototypes and function definitions in the same file just doesn't make sense. Just declare and define them before their use.
  • Don't use using namespace std;: you are basically defeating the purpose of namespace. Get used to the std:: prefix instead.
  • Don't use srand or rand for uniform distributions: use std::uniform_int_distribution<int> instead.
  • When you are using variables postfixed with incrementing numbers (like roll1, roll2, roll3...), it's probably a good idea to use an std::array or std::vector instead.
  • When you are duplicating a lot of code (like all those identical switch statements), it's usually the sign that you either need a loop or a function.
Improve this answer
\$\endgroup\$
4
\$\begingroup\$

Code Review is for reviewing existing code and not to discuss what additional features to add so I'm going to focus on reviewing what is there. This should reduce the amount of existing code somewhat and make it less daunting to add new functionality.

  1. Your functions and variables not always very well named. The name of a variable or function should convey its purpose in a concise manner so that when reading the code it is immediately obvious what the intended purpose is. So for example:

    • getNum should probably be named getRandomDieRoll
    • The diceX methods should be named drawDieX

  2. You have tremendous amounts of code repetition. Programming is about efficiently solving a problem in an automated way and you should also try to apply this to the code your write. One of the most important principles is: DRY - Don't Repeat Yourself.

Now let's see how we can start refactoring things

First of all the switch blocks all have the same code so we can easily extract those into a function:

void drawDieForRoll(int roll)
{
    switch (roll)
    {
        case 1: drawDie1(); break;
        case 2: drawDie2(); break;
        case 3: drawDie3(); break;
        case 4: drawDie4(); break;
        case 5: drawDie5(); break;
        case 6: drawDie6(); break;
    }
}

Now we can simplify diceRoll to:

void diceRoll()
{
int roll1=0;
int roll2=0;
int roll3=0;
int roll4=0;
int roll5=0;
char selection='y';
bool re1=' ';
bool re2=' ';
bool re3=' ';
bool re4=' ';
bool re5=' ';
char choice1=' ';
char choice2=' ';
char choice3=' ';
char choice4=' ';
char choice5=' ';
do{
cout<<"roll dice"<<endl;
roll1=getNum();
roll2=getNum();
roll3=getNum();
roll4=getNum();
roll5=getNum();

drawDieForRoll(roll1);
drawDieForRoll(roll2);
drawDieForRoll(roll3);
drawDieForRoll(roll4);
drawDieForRoll(roll5);

    cout<<"would you like to re-roll die 1 (y/n)"<<endl;
    cin>>choice1;
    if(choice1=='y')
    {
        re1=true;
    }
    if(re1=true)
    {
        roll1=getNum();     
    }//endif
    if(choice1=='n')
    {
        cout<<roll1;
    }


cout<<"would you like to re-roll die 2 (y/n)"<<endl;
cin>>choice2;
if(choice2=='y')
{
    re2=true;
}
    if(re2=true)
    {
    roll2=getNum();
    drawDieForRoll(roll2);
    }//endif

cout<<"would you like to re-roll die 3 (y/n)"<<endl;
cin>>choice3;
if(choice3=='y')
{
    re3=true;
}
    if(re3=true)
    {
    roll3=getNum();
    drawDieForRoll(roll3);
    }//endif

cout<<"would you like to re-roll die 4 (y/n)"<<endl;
cin>>choice4;
if(choice4=='y')
{
    re4=true;
}
    if(re4=true)
    {
    roll4=getNum();
    drawDieForRoll(roll4);
    }//endif

cout<<"would you like to re-roll die 5 (y/n)"<<endl;
cin>>choice5;
if(choice5=='y')
{
    re5=true;
}
    if(re5=true)
    {
    roll5=getNum();

    drawDieForRoll(roll5);
    }//endif

cin>>selection;
system("cls");
}while(selection=='y');

}

This reduced the function from 365 lines down to 100 lines, not bad.

Now we can see that the block asking to re-roll die X are basically also all the same except for the die number.

Now let's store the value of the rolls in an array which holds one entry per die. And because this is C++ we are going to use a std::vector<int>

const int NumberOfDice = 5;

std::vector<int> rolls(NumberOfDice);

Now you can use a nice loop to roll your dice and diceRoll can be further simplified into:

void diceRoll()
{
    char selection='y';

    const int NumberOfDice = 5;
    std::vector<int> rolls(NumberOfDice);

    do {
        cout << "roll dice" << endl;

        for (int dieIndex = 0; dieIndex < NumberOfDice; ++dieIndex)
        {
            int currentRoll = getRandomDieRoll();
            rolls[dieIndex] = currentRoll;

            drawDieForRoll(currentRoll);
        }

        for (int dieIndex = 0; dieIndex < NumberOfDice; ++dieIndex)
        {
            char choice = ' ';
            cout << "would you like to re-roll die " << (dieIndex + 1) << " (y/n)"<<endl;
            cin >> choice;
            if (choice == 'y')
            {
                int currentRoll = getRandomDieRoll();
                rolls[dieIndex] = currentRoll;
                drawDieForRoll(currentRoll);
            }
        }

        cin >> selection;
        system("cls");
    } while (selection == 'y');        
}

We just got rid of another 65 lines of code so we have removed 90% of all code from that function retaining the same functionality.

The next step is to simplify your drawing functions. If you want to draw ASCII art then well, just draw it:

void drawDie1()
{
    cout << "*********\n"
         << "*       *\n"
         << "*   X   *\n"
         << "*       *\n"
         << "*********\n";
}

void drawDie2()
{
    cout << *********\n"
         << *   X   *\n"
         << *       *\n"
         << *   X   *\n"
         << *********\n";
}

void drawDie3()
{
    cout << *********\n"
         << *   X   *\n"
         << *   X   *\n"
         << *   X   *\n"
         << *********\n";
}

void drawDie4()
{
    cout << *********\n"
         << * X   X *\n"
         << *       *\n"
         << * X   X *\n"
         << *********\n";
}

void drawDie5()
{
    cout << *********\n"
         << * X   X *\n"
         << *   X   *\n"
         << * X   X *\n"
         << *********\n";
}

void drawDie6()
{
    cout << *********\n"
         << * X   X *\n"
         << * X   X *\n"
         << * X   X *\n"
         << *********\n";
}

These changes will be removing another 130 lines of code and make it much nicer to read as well.

Will all of the above the code is now about 80-90% shorter than before and it should be much easier to handle and add new functionality.

Disclaimer: I haven't compiled it so there might be the one or other typo hidden but hopefully you should be able to get it to work.

Improve this answer
\$\endgroup\$
2
\$\begingroup\$

You might consider not generating the dice displays every time, but rather storing them as strings, for example:

To display a dice value of 1 you could use the string:

std::string displayValue1 = "\n***\n*x*\n***\n\n";

Then you could store your strings in an std::array or an std::vector which you index instead of using your switch statements.

std::vector<std::string> vectorOfDisplays
{
    displayValue1,
    displayValue2,
    displayValue3,
    displayValue4,
    displayValue5,
    displayValue6
};

For example instead of:

switch(roll1)
{
case 1:
    dice1();
break;

case 2:
    dice2();
break;

case 3:
    dice3();
break;

case 4:
    dice4();
break;

case 5:
    dice5();
break;

case 6:
    dice6();
break;

}//endSwitch

You could then do:

cout << vectorOfDisplays.at(roll1);

Alright... not quite, you might need to have roll1-1 so that it is an index into a container (0-5) instead of (1-6).

Doing so would probably reduce some of the complexity of your code.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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