HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Roulette game in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
gameroulettestackoverflow

Problem

I'm kind of new to C++ and was just wondering if anyone could give me tips on how I could make my code more efficient (I added some comments to the code to help you understand it better).

Is it a good idea to use switch cases? Is there a way I can use more functions/arrays/pointers?

Source code

Here is some of the code:

case 1:
        //Asks for number to place bet on
        cout > betNumber;

        //Checks if number is valid (between 1 and 36)
        while (betNumber  36) {
            cout > betNumber;
        }

        //Asks for amount to bet on that number
        cout > betAmount;

        //Checks if minimum amount is $1 and if the player has enough money in their account
        while (betAmount  bankAccount) {
            cout > betAmount;
        }

        //Seeds random number to the generator
        srand(time(0));

        //Generates a random number
        randomNumber = 1 + (rand() % 36);

        cout << endl << "The ball landed on the number " << randomNumber << ".";

        //Checks if player won or lost their bet
        if (betNumber == randomNumber) {
            bankAccount = win(betAmount, betOdds, bankAccount);
            cout << endl << "Congratulations, you won! You now have $" << bankAccount << " in your account." << endl;
        } else {
            bankAccount = lose(betAmount, bankAccount);
            cout << endl << "Bad luck, you lost! You now have $" << bankAccount << " in your account." << endl;
        }

        break;

Solution

Since there aren't any loops in your code that aren't waiting for the user to provide some input, it's probably way premature to talk about efficiency. Is your code measurably slow? Does it leave you hanging? If not, unless you have an unusual need, save questions on efficiency for later.

So let's talk about some more important things: readability and maintainability. Here are some tips on ways to improve the readability and maintainability of your code. These aren't hard rules that you should never break (especially the one on comments); they are guidelines on ways to make your life easier that you should learn to bend when the guideline makes things awkward instead.

I hope this helps, even though it may be a lot to take in all at once. Feel free to ask follow-up questions and get other opinions.

Avoid Redundancy

Redundancy can show up in many forms. One example of it is in your declaration of arrays, using code like int betType[11] = {35, 17, 8, 11, 5, 2, 1, 1, 2, 1, 1};. Unless there's something very special about the number 11, there's no reason to call it out. Instead just say int betType[] = {35, 17, 8, 11, 5, 2, 1, 1, 2, 1, 1}; which will automatically determine the size of the array for you.

When you later check your bounds with while (betChoice 11) {, you can instead use a calculated size (_countof(betType) or sizeof(betType)/sizeof(betType[0])), or even use a std::vector instead of an int[], and check against the vector's size().

This will help you avoid magic numbers that don't mean much later. After all, if someone asks you what's special about 11, would you think it's the number of available bet types? But if they asked what betTypes.size() meant, it would be easy to answer.

Another way redundancy shows up is in large blocks of repeating code. For instance, case 1 and case 2 have almost the same code. In fact I had to read it a couple times to find the part that was different. Sometimes this can be best handled by refactoring similar parts of code into functions, and passing parameters to them that control how they differ. Sometimes it's better just to extract the parts that are identical into simpler functions, and use them. I'll touch on this more below, but I certainly don't have the answer.

Avoid Obfuscation

In the code commented Displays a large dollar sign, there are a lot of casts from int to char so that cout prints the value as a character. But the characters in question are not that unusual. Just use the actual character you want to show, for example replacing

cout << endl << "       " << (char)36 << (char)36 << (char)36 << (char)36 << (char)36;


with

cout << endl << "       $$$"


This will not only be easier to type or update, it will be easier to read.

Avoid Comments

This recommendation is somewhat controversial, but it begins to target your question about functions. Instead of commenting what a line of code does, comment how a block of code does something unusual. When you're first starting out, everything seems unusual, but eventually you will see patterns and only need to comment on things that are not common patterns.

But then, instead of commenting what a block of code does, give it a name instead by putting it in a function. For example, you have several cases where you ask how much the user wants to bet on a number, then loop until they enter a valid number. You could extract this loop into a helper function like this:

int getBetAmount(int bankAccount)
{
    int betAmount;
    cin >> betAmount;
    while (betAmount  bankAccount)
    {
        cout > betAmount;
    }
    return betAmount;
}

int _tmain() {
    : : :
    case 1:
        : : :
        cout << endl << "How much would you like to bet on the number" << betNumber << "? $";
        betAmount = getBetAmount(bankAccount);
        : : :
    : : :
    case 2:
        : : :
        cout << endl << "How much would you like to bet on the numbers" << betNumber << " and " << betNumber + 3 << "? $";
        betAmount = getBetAmount(bankAccount);
        : : :
}


Find some other code that doesn't change much and extract that into functions as well. For example, the code commented Checks if player won or lost their bet, I see creating a function you'd call like this:

case 1:
    : : :
    bankAccout = awardWinnings(betNumber == randomNumber, betAmount, betOdds, bankAccount);
    break;
case 2:
    : : :
    bankAccount = awardWinnings(betNumber == randomNumber || betNumber + 3 == randomNumber, betAmount, betOdds, bankAccount);
    break;


After you make these changes, ideally the parts that are different will start to stand out, and the parts that are the same will have good names that tell you what they do even if they don't have a comment. And then you can more easily avoid incorrect comments like case 2's Check if number is valid (between 1 and 36) that actually checks for 33.

You can also avoid comments by naming constants. Instead of star

Code Snippets

cout << endl << "       " << (char)36 << (char)36 << (char)36 << (char)36 << (char)36;
cout << endl << "       $$$$$"
int getBetAmount(int bankAccount)
{
    int betAmount;
    cin >> betAmount;
    while (betAmount < 1 || betAmount > bankAccount)
    {
        cout << endl << "You have $" << bankAccount << " in your bank account: $";
        cin >> betAmount;
    }
    return betAmount;
}

int _tmain() {
    : : :
    case 1:
        : : :
        cout << endl << "How much would you like to bet on the number" << betNumber << "? $";
        betAmount = getBetAmount(bankAccount);
        : : :
    : : :
    case 2:
        : : :
        cout << endl << "How much would you like to bet on the numbers" << betNumber << " and " << betNumber + 3 << "? $";
        betAmount = getBetAmount(bankAccount);
        : : :
}
case 1:
    : : :
    bankAccout = awardWinnings(betNumber == randomNumber, betAmount, betOdds, bankAccount);
    break;
case 2:
    : : :
    bankAccount = awardWinnings(betNumber == randomNumber || betNumber + 3 == randomNumber, betAmount, betOdds, bankAccount);
    break;

Context

StackExchange Code Review Q#35248, answer score: 5

Revisions (0)

No revisions yet.