Text-based dice rolling game in C++

Question

I have made this small text-based dice rolling game in C++. How could I improve it? Be as picky as you'd like.

#include <iostream>
#include <cstdlib>
#include <ctime>
using namespace std;
void mainGame(int &numDice);
int main()
{ 
 srand (time(NULL));
 int gameRunning = true;
 int numDice;
 while (gameRunning == true)
 {
 system("cls");
 cout << "\n How many dice would you like to use? (1-4)" << endl;
 cout << "\n Enter '0' to exit." << endl;
 cout << "\n> ";
 cin >> numDice;
 if (numDice == 1 || numDice == 2 || numDice == 3 || numDice == 4) 
 {
 mainGame(numDice);
 }
 else if (numDice == 0)
 {
 return 0;
 }
 else {
 system("cls");
 cout << "\n That was invalid input!" << endl;
 cout << "\n ";
 system("pause");
 }
 }
}
void mainGame(int &numDice)
{
 bool goAgain = true;
 int dice[4];
 int input;
 while (goAgain == true)
 {
 for (int x = 0; x != numDice; x++)
 {
 dice[x] = (rand() % 6) + 1;
 cout << "\n Dice " << x + 1 << " shows a " << dice[x] << "!" << endl;
 cout << "\n ";
 system("pause"); 
 }
 system("cls");
 cout << "\n Would you like to go again with " << numDice << " dice?" << endl;
 cout << "\n Enter '1' for yes or '2' for no." << endl;
 cout << "\n> ";
 cin >> input;
 switch (input)
 {
 case 1:
 break;
 case 2:
 goAgain = false;
 }
 }
}

• Der Kommissar: Define improve, are you looking for performance or readability?
• Elliot Morgan: Either one really
• Der Kommissar: As far as readability, you could reduce numDice == 1 || numDice == 2 || numDice == 3 || numDice == 4 to numDice >= 1 && numDice <= 4. This will also technically speed it up a minuscule amount of numDice is 0, 3, 4, 5 or 6. That's a pretty quick and easy one.
• Der Kommissar: That's about the best I can do - I'm no C++ expert. (Hence why I didn't post it as an answer.) I can't see anything else that's glaring, however. (Except for the cout lines being a bit long, but that's not necessarily your fault.)
• Mast: Is this homework and if so, would a OO approach score bonus points?

Answer 1

using namespace std; is considered a bad practice.

---

Don't do this:

int gameRunning = true;

If you want to use a boolean variable, use a bool instead of int.

---

You can use boolean values directly in conditions, no need to compare them to true / false. So instead of this:

while (gameRunning == true)

You can write simply:

while (gameRunning)

---

Still about that while loop earlier, since gameRunning is initialized to true, the loop condition will be inevitable true for the first time. That's kind of a waste to re-evaluate. In situations like this, considering rewriting it as a do { ... } while (...) loop.

• Elliot Morgan: Making gameRunning an int was an accident. I'll make the other changes too. Thanks.

Answer 2

This is pretty good code for a first draft. Everything you've written is easy to understand. I didn't have to guess what anything was doing, which is awesome!

Most of my suggestions are nitpicky and may seem a bit grandiose for a small program like this, but it's worth getting into the habit of doing these things because programs often grow quickly and it can be hard to manage that change when they do.

Use The Right Function

rand() is a terrible random generator. The man page for rand() describes it this way:

RAND(3) BSD Library Functions Manual RAND(3)

NAME rand, rand_r, srand, sranddev -- bad random number generator

You should try something like arc4random() or some other better random number generator.

Group Functionality Into Functions

I would make this entire block a separate function named something like displayInstructions():

 system("cls");
 cout << "\n How many dice would you like to use? (1-4)" << endl;
 cout << "\n Enter '0' to exit." << endl;
 cout << "\n> ";

Likewise with the error text. I'd put it into a function named something like displayError():

 system("cls");
 cout << "\n That was invalid input!" << endl;
 cout << "\n ";
 system("pause");

It's likely that in the future there may be more than 1 type of error. (In fact there are already 2 places where the input can be invalid - when getting the number of dice and when deciding to run again or not.)

The mainGame() function actually does 2 different things. It displays the dice rolls, and then it determines if the user wants to roll again. I would split out the second part into a function named something like rollAgain(). And to make it simpler, I'd probably remove the case statement and just use a couple of ifs. And if you want to handle errors, you'll need to have a loop in there:

bool rollAgain()
{
 for (;;)
 {
 system("cls");
 cout << "\n Would you like to go again with " << numDice << " dice?" << endl;
 cout << "\n Enter '1' for yes or '2' for no." << endl;
 cout << "\n> ";
 cin >> input;
 // If the input is 1 or 2 it's valid, so return true if it's 1, false otherwise
 if ((input == 1) or (input == 2))
 {
 return input == 1;
 }
 // The input was not valid, display an error and ask for input again
 displayError();
 }
}

Naming Is The Hardest Part

It's said that the hardest part of computer programming is naming things. The name mainGame() doesn't say a whole lot. I'd name it something more descriptive like rollDice(). Because eventually, you'll probably want to use the results of those dice rolls to do something more interesting than just display them to the user.

If You Aren't Going To Change It, Make It Const

In mainGame(), the numDice argument isn't changed. It should be const int numDice in that case.

Magic Numbers

And speaking of constants, I notice that you have several numbers hard-coded in your code. Generally, it's a good idea to make those into named constants so you or other people can remember what they represent. So instead of using 1 and 4 in your if statement, I'd make 2 constants like this:

const int MIN_NUM_DICE = 1;
const int MAX_NUM_DICE = 4;

and the if would look like this:

if ((MIN_NUM_DICE <= numDice) and (numDice <= MAX_NUM_DICE))
{
 mainGame(numDice);
}

This way, if you ever decide you want more dice rolls you change the constant in one place, and if it's a value that's used in multiple places you don't have to change it everywhere it's used.

I'd also change the 6 in mainGame() to a constant. Something like:

const int NUM_SIDES_PER_DIE = 6;

Maybe someday you'll want to support 12-sided or 20-sided die, for example. If you do, then it's a lot easier to understand that the rand call is you "dice roll": dice[x] = (rand() % NUM_SIDES_PER_DIE) + 1;

User Interface

And if I'm being really nit-picky, instead of using 1 and 2 to choose whether or not to play again, make it a yes/no question and let the user enter y(or Y) or n(or N).

• Hurricane: arc4random is not portable. It's BSD (and thus OSX). srand/rand is perfect for a small game like this one. (A cryptographically secure algorithm may not be worth the loss of portability to generate numbers from 1 to 6).
• user1118321: @Hurricane - I totally agree. As I said, these suggestions are nit-picky! But I do think it's worth learning a little about these things while you're learning to program, so I thought I'd mention it. Definitely not a deal-killer.

Answer 3

I see others have warned you about using namespace std;.

Use of platform-dependent calls

In general they're not a good idea. If you have to port your code to another platform that doesn't support cls and/or pause you'd have to go through and change all instances. I'd prefer to use an existing library that abstracts away the platform-dependent calls. Next best would be to put all the platform-dependent code in its own routines, and then call that routine when you need. That way you only need to change the code in one place.

Pointless variables

gameRunning starts out as true and is never changed. Lose it.

Appropriate use of `endl`

The purpose of std::endl is to emit a newline and make sure that all output has been flushed, like just before you perform a long-running process. It's not needed in any of the cases you're using it. (any use of cin automatically flushes output). But it is needed right before you call system("pause") (otherwise the prompt or newline may not show up). In most cases, just use "\n".

`const`-correctness

Since mainGame() doesn't change numDice, you should mark the parameter as const.

Magic numbers

int dice[4];

Why 4 here? What if you want to expand the maximum number of dice later? The 4 should be declared as a const int maxDice in the appropriate scope, so that it can be used everywhere you want to see the maximum.

Unexpected input

What do you want the code to do if the user types in a non-integer value at any of the calls to cin? Or what if they type in something other than 1 or 2?

Separate business logic and IO

The mainGame function both performs the business logic of assigning dice, and input/output. It's best for these to be in separate functions.

What's the `dice[]` array for again?

I don't see any point to putting the dice rolls into an array. Were you supposed to build the whole array first and then print it out?

Answer 4

This loop

while (goAgain == true)
{
 //...
 switch (input)
 {
 case 1:
 break;
 case 2:
 goAgain = false;
 }
}

can be written as

for (;;) //Infinite loop
{
 //...
 if(input == 2)
 break;
}

thereby, shortening your code and also eliminating the need of the goAgain variable. The same thing can be done for the other loop:

while (gameRunning == true)
{
 system("cls");
 cout << "\n How many dice would you like to use? (1-4)" << endl;
 cout << "\n Enter '0' to exit." << endl;
 cout << "\n> ";
 cin >> numDice;
 if (numDice == 1 || numDice == 2 || numDice == 3 || numDice == 4) 
 {
 mainGame(numDice);
 }
 else if (numDice == 0)
 {
 return 0;
 }
 else {
 system("cls");
 cout << "\n That was invalid input!" << endl;
 cout << "\n ";
 system("pause");
 }
}

to eliminate the need for gameRunning:

for (;;) //Infinite loop
{
 system("cls");
 cout << "\n How many dice would you like to use? (1-4)" << endl;
 cout << "\n Enter '0' to exit." << endl;
 cout << "\n> ";
 cin >> numDice;
 if (numDice >= 1 && numDice <= 4) //As suggested by @ElliotMorgan 
 {
 mainGame(numDice);
 }
 else if (numDice == 0)
 {
 break;
 }
 else {
 system("cls");
 cout << "\n That was invalid input!" << endl;
 cout << "\n ";
 system("pause");
 }
}

Another thing I see is that you use pass-by-reference for numDice to be passed to mainGame. But main doesn't seem to use its value anymore. I suggest using pass-by-value instead.

• Snowbody: It's preferable to use for(;;) instead of while(1) or while(true) since it avoids an arbitrary constant.
• Morwenn: @Snowbody I'd say it's more of a matter a matter of style.

Answer 5

Nice job. If you were looking to add a little more complexity to it as a learning experiment I'd recommend using dynamic memory for your dice function, rather than limiting the user to 5 dice.

Answer 6

Check out this link on the using directive <using namespace std> Placing a using directive in the global namespace (which you have done) contradicts the value of namespaces. Consider placing using definitions within blocks. Placing using definitions within blocks puts them in the local scope. For example:

void somefunction()
{
 using std::cout;
 using std::cin;
 //some code...
} 

Just like when a local variable losses scope when a program leaves the block, so does a namespace. This not only documents which names you use but also allows you to omit names you don't use. Even placing such using definitions at the start of a file is preferable to using an entire namespace such as using namespace std; You could also qualify the name with the scope resolution operator. For example:

std::cout << "Hello";
std::cin >> var;

This seems to be the prevailing method for many people. You decide which is best.

• Elliot Morgan: The fact that using 'using namespace std;' is bad practice has always confused me but the link has helped me understand it, so thank you.