Skip to main content
3 of 4
fixed variable name and grammar
Edward
  • 66.6k
  • 4
  • 118
  • 282

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.

Edward
  • 66.6k
  • 4
  • 118
  • 282