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

Fruitmachine game

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

Problem

I'm quite happy with my code so far but I often make efficiency errors that I don't see when writing code. Have I missed anything?

```
#include
#include

int main ()
{
char end = 'N';
int credit = 5, bet = 0, userInput = 0, cont = 0, victory;

do{
while (cont == 0){
// Introduction of credit system to user
printf("\e[1;1H\e[2J");
printf("Your current credits are %d.\n", credit);
printf("How many credits would you like to bet?\n");
scanf("%d", &userInput);
printf("\e[1;1H\e[2J");
// Checking for valid credit input and adjusting credit/bet value according to input
if (userInput = 1){
bet = userInput;
credit = credit - bet;
cont = 1;
printf("You have bet %d credits. Good luck!\n", bet);
sleep(2);
break;
}
if (userInput == 0){
printf("Invalid, bet must be a minimum of 1 credit.\n");
sleep(2);
cont = 0;
}
else{
printf("Invalid, input exceeded your credit value of %d.\n", credit);
sleep(2);
cont = 0;
}
// End of check
}

// Introduction countdown to execute after valid bet is placed
printf("\e[1;1H\e[2J");
printf("Ready?\n");
sleep(1);
printf("\e[1;1H\e[2J");
printf("3...\n");
sleep(1);
printf("\e[1;1H\e[2J");
printf("2...\n");
sleep(1);
printf("\e[1;1H\e[2J");
printf("1...\n");
sleep(1);
printf("\e[1;1H\e[2J");
printf("Spin!\n");
printf("\e[1;H\e[2J");

// Declaring var names for reel and loop
int firstReel, secondReel, thirdReel, loop = 0;

// Loop to print cycle reel 10 times
while(loop 10 && firstReel == secondReel || secondReel == thirdReel){
printf("Congratulations! You win! ");
victory = 1;
}
else
{
printf("Sorry, you lose. ");
victory = 0;
}

// Payout calculations according to victory value
if (victory == 1){
credit = credit + (bet * 2);
printf("Your credit is now %d.\n", credit);
}
else
{
if (credit == 0){
printf("Your credit is now 0. Ga

Solution

I see some things that may help you improve your code.

Don't use non-ISO escape sequences

I assume from the context that you are intending \e to mean the ESC character, '\x1b'. That might be supported by your compiler, but it's non-standard. Better would be to use '\x1b'.

Isolate ANSI escape sequences into functions

It's often good practice to split out your escape sequences into a separate function. It's easier to read and understand clearscreen() than printf("\e[1;1H\e[2J"); for most people.

Use all the required #include files

The code calls srand and time but doesn't include the corresponding files. The code should have these two lines added:

#include 
#include 


Eliminate needless repetition

The switch statements that print the names of the icons could be much simplified by adding this at file scope.

static const char* icon[4] = {"Bell", "Cherry", "Orange", "Horseshoe"};


And then replacing all three switch statements with this one line:

printf("%s %s %s\n", icon[firstReel], icon[secondReel], icon[thirdReel]);


Don't reseed the random number generator more than once

The program currently calls srand at the top of each loop, but this is really neither necessary nor advisable. Instead, just call it once when the program begins and then continue to use rand() to get random numbers.

Separate input, output and calculation

To the degree practical it's usually good practice to separate input, output and calculation for programs like this. By putting them in separate functions, it isolates the particular I/O for your platform (which is likely to be unique to that platform or operating system) from the logic of the game (which does not depend on the underlying OS).

Be careful with signed versus unsigned numbers

If you only want to allow positive numbers for credit and bet which makes sense in this context, they should be declared as unsigned rather than int. Similarly, you can use %u in printf and scanf statements rather than %d.

Prefer control constructs to state variables

Using state variables such as cont tends to lead to fragile code that's hard to debug and maintain. Better would be to make it dependent on a necessary variable such as bet. So for instance, you could define a function as follows:

static unsigned getBetAmount(unsigned credit)
{
    unsigned bet = 0;
    while (bet == 0) {
        // Introduction of credit system to user
        printf("\x1b[1;1H\x1b[2J");
        printf("Your current credits are %u.\n", credit);
        printf("How many credits would you like to bet?\n");
        scanf("%u", &bet);
        printf("\x1b[1;1H\x1b[2J");
        // validate bet amount
        if (bet > credit) {
            bet = 0;
            printf("Invalid, input exceeded your credit value of %d.\n", 
                   credit);
        }
        else if (bet == 0) {
            printf("Invalid, bet must be a minimum of 1 credit.\n");
        }
        else {
            printf("You have bet %d credits. Good luck!\n", bet);
        }
        sleep(2);
        // End of check
    }
    return bet;
}


Then the first two lines of your big loop could be this:

bet = getBetAmount(credit);
credit -= bet;


Avoid checking state multiple times

The logic of the game currently checks the state of the victory variable multiple times, when really all that's needed is to either register a win or a loss. If you were to separate out a spin function which does the looping and displaying of the reels and simply returns true if the user has won, the win/loss calculations are much simplified:

// Win/lose conditions
if (spin()) {
    printf("Congratulations! You win! ");
    credit = credit + (bet * 2);
    printf("Your credit is now %d.\n", credit);
}
else
{
    printf("Sorry, you lose. ");
    if (credit == 0) {
        printf("Your credit is now 0. Game over.\n");
        return 0;
    }
}


Try to consolidate loop exit conditions at the top

Prefer a for or while loop to do {...} while() loop. Because the reader is forced to search for the loop terminating condition, it's more effort to understand the latter construct. This code, for example could be structured like this:

for (char end = 'N'; end == 'N'; )
{
    bet = getBetAmount(credit);
    credit -= bet;
    countdown();
    //  all of the rest of the code
}


This also eliminates a break statement. A break can also contribute to difficulties understanding a loop because again, the programmer reading the code has to search for them.

Eliminate spurious variables

There are several variables, including cont, victory, userInput, rndOne, rndTwo and rndThree which can be elmininated from the code. Reducing the number of variables, especially when they're not really needed can make your code easier to understand and easier to maintain and debug.

Omit return 0 at the end of main

The compiler will automatically ge

Code Snippets

#include <stdlib.h>
#include <time.h>
static const char* icon[4] = {"Bell", "Cherry", "Orange", "Horseshoe"};
printf("%s %s %s\n", icon[firstReel], icon[secondReel], icon[thirdReel]);
static unsigned getBetAmount(unsigned credit)
{
    unsigned bet = 0;
    while (bet == 0) {
        // Introduction of credit system to user
        printf("\x1b[1;1H\x1b[2J");
        printf("Your current credits are %u.\n", credit);
        printf("How many credits would you like to bet?\n");
        scanf("%u", &bet);
        printf("\x1b[1;1H\x1b[2J");
        // validate bet amount
        if (bet > credit) {
            bet = 0;
            printf("Invalid, input exceeded your credit value of %d.\n", 
                   credit);
        }
        else if (bet == 0) {
            printf("Invalid, bet must be a minimum of 1 credit.\n");
        }
        else {
            printf("You have bet %d credits. Good luck!\n", bet);
        }
        sleep(2);
        // End of check
    }
    return bet;
}
bet = getBetAmount(credit);
credit -= bet;

Context

StackExchange Code Review Q#70102, answer score: 10

Revisions (0)

No revisions yet.