11
\$\begingroup\$

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

#include <iostream> #include <cstdlib> #include <ctime> using namespace std; // General functions void bankrupt(); // Roulette functions void roulette01(); void roulette02(); void rouletteWin(); void rouletteLoss(); // Slots functions void slots01(); void slots02(); // General variables int invalid = 1; int input; int dollars = 50; // Roulette variables int bet = 0; char cColour; int iColour; int actualColour; // Slots variables int randSlots01; int randSlots02; int randSlots03; int main() { srand (time(NULL)); while (invalid == 1) { system("cls"); cout << "\n Welcome to the Casino! What would you like to play?" << endl; cout << "\n 1. Roulette" << endl; cout << "\n 2. Slots" << endl; cout << "\n 3. Exit" << endl; cout << "\n> "; cin >> input; switch (input) { case 1: input = 1; roulette01(); case 2: slots01(); case 3: exit(0); default: cout << "\n That was invalid input!" << endl; cout << "\n "; system("pause"); invalid = 1; } } } void roulette01() { if (dollars <= 0) { bankrupt(); } system("cls"); cout << "\n Roulette" << endl; cout << "\n You have " << dollars << " dollars!" << endl; cout << "\n Enter 1 to place a bet. Enter 2 to exit to the main menu." << endl; cout << "\n> "; cin >> input; switch (input) { case 1: roulette02(); case 2: main(); } } void roulette02() { cout << "\n Enter how much you would like to bet." << endl; cout << "\n> "; cin >> bet; if (bet > dollars) { cout << "\n You don't have enough money for that bet!" << endl; cout << "\n "; system("pause"); roulette02(); } cout << "\n Would you like to bet on red or black?" << endl; cout << "\n Enter r for red. Enter b for black." << endl; cout << "\n> "; cin >> cColour; if (cColour == 'r') { iColour = 1; } else { iColour = 2; } actualColour = rand() % 2; if (actualColour == iColour) { rouletteWin(); } else { rouletteLoss(); } } void rouletteWin() { dollars = dollars + bet; cout << "\n You won " << bet << " dollars!" << endl; cout << "\n "; system("pause"); roulette01(); } void rouletteLoss() { dollars = dollars - bet; cout << "\n You lost " << bet << " dollars!" << endl; cout << "\n "; system("pause"); roulette01(); } void slots01() { if (dollars <= 0) { bankrupt(); } system("cls"); cout << "\n Slots" << endl; cout << "\n You have " << dollars << " dollars!" << endl; cout << "\n Enter 1 to play. Enter 2 to exit to the main menu." << endl; cout << "\n> "; cin >> input; switch (input) { case 1: slots02(); case 2: main(); } } void slots02() { randSlots01 = rand() % 3; randSlots02 = rand() % 3; randSlots03 = rand() % 3; if (randSlots01 == randSlots02 && randSlots02 == randSlots03) { dollars = dollars + 10; cout << "\n You won 10 dollars!" << endl; cout << "\n "; system("pause"); slots01(); } else { dollars--; cout << "\n You lost 1 dollar!" << endl; cout << "\n "; system("pause"); slots01(); } } void bankrupt() { dollars = 50; cout << "\n You have gone bankrupt! Time to start over." << endl; cout << "\n "; system("pause"); main(); }
\$\endgroup\$

2 Answers 2

Reset to default
13
\$\begingroup\$
  • Your naming needs a lot of work. In particular it's a very bad idea to name your entities with numeric suffixes, as that doesn't explain at all what they're doing (it shows they're related but not how, and definitely not how they differ from each other in function).
  • All your variables are global. This is very bad form. Variables should have the smallest possible scope, so it's plainly evident where they're used and what they're used for. There are a number of ways to deal with global state; but at this level there probably should just be a struct that gets passed around.
  • You've got some unnecessary code. There's no reason for input = 1; in the first case.
  • You shouldn't use exit(0) unless you have a reason to. return works perfectly well for returning from main().
  • Don't rely on system() for controlling the user interface. It makes your code impossible to port. At the very least, make helper functions for each of the tasks so that you just have to replace the code once in each helper function.
  • Your program crashes if the user does not input an integer at the main menu.
  • The sub-functions, roulette01() and slots01(), should not call main(). main() already has a loop in it, so you can just return; from them to get back to main().
  • Similarly, roulette01() and slots01() should be structured to include a loop, so that you don't have functions recursively calling each other.
  • Ideally you ought to completely separate the logic of each game from the user interface. One function does the logic, another function calls it and displays the results.
\$\endgroup\$
13
\$\begingroup\$

I have found a couple of things that could help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It's an alarmingly common thing for new C++ programmers to do.

Don't call main

According to the C++ ISO standard section 3.6.1

The function main shall not be used within a program.

So your program is technically malformed. Rather than doing it that way, simply return to main if needed.

Avoid the use of global variables

I see that invalid is used only within main but it's declared as s global variable. It's generally better to explicitly pass variables your function will need or declare them within the appropriately smallest possible scope rather than using the vague implicit linkage of a global variable.

Don't use system("pause")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example:

void pause() { getchar(); }

Use a menu object or at least a common menu function

In a number of places in your code, you have something like a menu. Your code presents a couple of options and then asks the user to pick one based on an input number. Rather than repeating that code in many places, it would make sense to make it generic. Only the prompt strings actually change, but the underlying logic of presenting the choices and asking for input are all the same. It looks like you're a beginning programmer, and so perhaps you haven't learned about objects yet, but this kind of repeated task with associated data is really well-suited to object-oriented programming and that's something that C++ is very good at expressing.

Use better function names

The names you've chosen are not too bad, but slots01 and slots02 are poor names. We can infer that they have something to do with slots, but what? The name should suggest that.

Restructure the code

The bankrupt() routine is called when the user attempts to play slots or roulette, but it would make more sense to put those calls within main.

Consider using a better random number generator

You are currently using 

actualColour = rand() % 2;

There are two problems with this approach. One is that the low order bits of the random number generator are not particularly random, so neither will actualColour be. On my machine, there's a slight but measurable bias toward 0 with that. The second problem is that it's not thread safe because rand stores hidden state. A better solution, if your compiler and library supports it, would be to use the C++11 `std::uniform_int_distribution. It looks complex, but it's actually pretty easy to use.

\$\endgroup\$
1
  • \$\begingroup\$ Regarding random number generation, I highly recommend Stephan Lavavej's (Microsoft) talk on why rand() is bad - he also shows how a novice can easily use the C++ standard library's tools as mentioned in the post. \$\endgroup\$ Commented Mar 30, 2015 at 21:39

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.