4
\$\begingroup\$

Ive been learning c for the past few weeks, I made a simple turn-based game to show off what I know. Is there anything I can do to improve the code or make the game run better?

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <windows.h>
#include <time.h>
#include <stdbool.h>

int main(){
    srand(time(NULL));
    bool play = true;
 
    while(play){
        int choose = -2;
        int P_h = 25;
        int E_h = 30;
        int SP_u = 3;
        puts("Do you wanna play this game. [Press 0 to exit or press any other number to play]");
        scanf("%d", &choose);
 
        if(choose == 0){
            play = false;
            puts("Goodbye");
            break;
        }
        else{
            puts("Okay");
            Sleep(300);
 
            while(E_h > 0){
                int P_at = (rand()%8);
                int E_at = (rand()%10);
                int SP = 100;
                int at_p = -5;
 
                if(P_h <= 0){
                    puts("You have died");
                    break;
                }
             if(E_h <= 0){
                puts("you win!");
                break;
             }
 
                printf("You have %d uses left of the super attack \n", SP_u);
                puts("Would you like to attack or do a super attack [Press 0 to do a super attack, Press 1 to attack]");
                scanf("%d", &at_p);
 
                if(at_p == 0){
                    if(SP_u <= 0){
                    puts("Sorry, you have 0 uses left, you cannot use this \n");
                }
                else{
                    E_h -= SP;
                    SP_u--;
                    printf("\nThe enemy took a damage of %d! It now has a health of %d!", SP, E_h);
 
                    P_h -= E_at;
                    printf("\nThe enemy attacks! Your health went down by %d it is now %d \n \n", E_at, P_h);
                }
 
            }
            else if(at_p == 1){
                 E_h -= P_at;
                printf("\nThe enemy took a damage of %d! It now has a health of %d!", P_at, E_h);
 
                P_h -= E_at;
                printf("\nThe enemy attacks! Your health went down by %d it is now %d \n \n", E_at, P_h);
            }
            else{
                puts("Thats not an option, idiot \n");
            }
 
        }
 
 
    }
    if(E_h <= 0){
            puts("You killed the enemy!");
            puts("Would you like to play again? [0 to exit, Press anything else to play again]");
            scanf("%d", &choose);
 
            if(choose == 0){
                play = false;
                break;
            }
            else{
                play = true;
            }
        }
        else if(P_h <= 0){
            puts("You died. Would you like to play again to try to defeat the enemy. [0 to exit, anything else to play again]");
            scanf("%d", &choose);
 
            if(choose == 0){
                play = false;
                break;
            }
            else{
                play = true;
            }
        }
    }
 
 
 
    return 0;
}

I feel like there is a better way to program the decisions because right now they are just nested if statements.

I think

\$\endgroup\$
0
3
\$\begingroup\$

Formatting & Conventions

Always run your code through an autoformatter. On Linux, I use clang-format, and on Win10 I use VSCode's autoformatting intellisense features. I'm not sure of your exact setup, but formatting is big.

Next, variable names. The variable choose is wonderfully descriptive. I know immediately what it's for. The variable P_h on the other hand is vague and undescriptive. Don't worry about how long your variable names are. The compiler won't care. Instead focus on making variable names that are descriptive, like Player_health so that anyone who reads your code immediately understands it.

Program Flow

Ignoring all the messy specifics, this is what I read your program's flow as:

while playing:
    setup variables.
    get player input
    if player doesn't want to play
        exit
    otherwise:
        main game loop
    if lose:
        ask to replay
    if win:
        ask to replay

I'm purposefully ignoring the main game loop for now. Right off the bat, I see one big problem: You do the same thing twice down at the bottom. That whole choose==0 thing can be moved outside the if statements because in either case, you'll encounter it. (In the case where E_h>0 && P_h>0, you won't have exited the main game loop, so we can ignore that.)

That would look like this:

if (E_h <= 0) {
    puts("You killed the enemy!\nWould you like to play again? [0 to exit, Press anything else to play again]");
}
else if (P_h <= 0) {
    puts("You died. Would you like to play again to try to defeat the enemy. [0 to exit, anything else to play again]");
}
scanf("%d", &choose);
if (choose == 0) {
    play = false;
    break;
}
else {
    play = true;
}

Even here, we can optimize further. Watching program flow, anytime we hit the scanf on this code, we can assert that play==true and so we can remove the whole section that sets it. Furthermore, since the very next thing we do is run while (play) we can cut out the break, since it'll exit the loop naturally. Finally, conditionals that only run a single statement don't need brackets. (That one's up to personal choice, and doesn't really affect your code. I like to knock them off of there.) Thus, we can take 25 lines of code down to

if (E_h <= 0)
    puts("You killed the enemy!\nWould you like to play again? [0 to exit, Press anything else to play again]");
else if (P_h <= 0)
    puts("You died. Would you like to play again to try to defeat the enemy. [0 to exit, anything else to play again]");
scanf("%d", &choose);
if (choose == 0)
    play = false;

just seven.


Next, let's look at the part of the code where you initially ask if the player wants to play. Logically, if the player runs your code, you can assume that they meant to. The whole initial choice section can be removed. (If you choose to leave it in there, think about what might happen if the player decides to replay the game. Right now, they have to say they want to (re)play the game twice!)

Main Game Loop

Ok, here's the meat of it. First thing you can do is move this little number to the end:

if (P_h <= 0) {
    puts("You have died");
    break;
}
if (E_h <= 0) {
    puts("you win!");
    break;
}

and remove the second break. That functions the same, but you don't have to calculate attack values for a round you're not going to use.

As for your actual 'attack' section, it looks like you can significantly reduce that as well. You've got two options here: Either use switch statements or modify the program flow. switch would look like:

switch at_p
{
    case 0: 
        super_attack code;
    break;
    case 1: 
        regular_attack code;       
    break;
    default:
        idiot_code;
}

As for program flow: Notice that in both of the attacks, you have very similar/identical code. If you code for the boundary conditions right, imagine what you could do with P_at = SP. You could significantly refactor your main loop.

I'd personally go for the switch statement simply because it's more amenable to future upgrades.

Other suggestions

Don't use stdbool.h. It's a lot easier to operate using int. Generally, false maps to 0 and true maps to anything that isn't false. (Plus, ints are normally 4 bytes big and bools are 1 byte big. You've got gigs of space to use.) I never use bools, and if I need to use them, I'll declare a union and make a utility variable that holds boolean flags.

Watch for program flow. A good maxim to work towards is having only one return location. Whether that's return, break, exit() or continue, minimizing the number of ways a program can escape a particular section of code makes it easier to debug. Oftentimes you can move things at the beginning of loops to the end, negate the conditional inside of an if statement, or otherwise simplify your code just by jimmying around with it a bit.

Keep track of variable's possible values throughout your code. If something has to be a certain value at a particular point in the code, then there's no point checking that value or setting it to what it already is. This can get a little more complicated with bigger code, but it's a powerful skill.

Suggestions for future features

Perhaps look at implementing some more options in the main game, such as

  • defending and armor
  • quitting from within the main game loop
  • breaking the attack/defend portions out into their own functions

Also take a look at

  • do-while loops
  • Switch statements
  • constants

All in all, your code is good, but there's a few modifications to program flow that can be made to cut down on operational complexity.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thank you! I will look into those functions. I cant believe that I didnt see that I could've used a switch statements. I will try to format/optimize my code better. \$\endgroup\$ – dareesome Apr 7 at 0:40
  • \$\begingroup\$ Overall agree, but discouraging stdbool is poor advice. There's a reason it exists - it allows programmers to better communicate their intent. This will produce equivalent code for the computer, but code is not only written for a computer - it's written for humans to read and modify, and an unconstrained, unaliased int makes this more difficult and less obvious. \$\endgroup\$ – Reinderien Apr 11 at 15:26
2
\$\begingroup\$

Much of this feedback overlaps with that of @JakobLovern who has already covered some great points; nevertheless:

  • You should break up main into subroutines
  • You can avoid play being needed if you have a subroutine that returns a boolean
  • Avoid aggressive abbreviation of your local variable names
  • Consider adding an input validation loop
  • Move your 'Goodbye' to a part of the logic that will be executed unconditionally on exit
  • Fix the issue where a player can have a negative health
  • Do not evaluate the enemy's turn if the enemy is already dead
  • Do not bother asking the user whether they want to run a super-attack if such an attack is not possible
  • Fix the double you-died output

Note that since I do not have Windows, I had to switch to a GNU-compatible standard library to run this. "Most" of this code is portable, and Windows-specific calls such as Sleep have been replaced.

Suggested

This was compiled via

gcc -Wall -std=c17 -D_GNU_SOURCE -o game game.c
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>     // rand
#include <time.h>       // time, nanosleep
#include <sys/param.h>  // MAX


static void sleep_ms(long delay) {
    const struct timespec ts = {.tv_sec=0, .tv_nsec=delay * 1000000};
    nanosleep(&ts, NULL);
}


static int ask_number(const char *prompt) {
    int choice;
    while (true) {
        puts(prompt);
        if (scanf("%d", &choice) == 1)
            return choice;

        // Clear the erroneous buffer
        // This is actually a little tricky to do in a reliable and portable manner; see
        // https://stackoverflow.com/questions/1716013/why-is-scanf-causing-infinite-loop-in-this-code
        int c;
        do {
            c = getchar();
        } while (c != '\n' && c != EOF);
    }
}


static bool ask_super(void) {
    while (true) {
        int super_choice = ask_number(
            "Would you like to attack or do a super attack [Press 0 to do a super attack, Press 1 to attack]"
        );
        switch (super_choice) {
            case 0: return true;
            case 1: return false;
            default:
                puts("That's not an option");
        }
    }
}


static bool play(void) {
    int player_health = 25,
        enemy_health = 30,
        super_uses = 3;

    while (true) {
        printf("You have %d uses left of the super attack\n", super_uses);
        int player_attack;
        if (super_uses > 0 && ask_super()) {
            player_attack = 100;
            super_uses--;
        }
        else
            player_attack = rand() % 8;

        enemy_health = MAX(0, enemy_health - player_attack);
        printf("The enemy took a damage of %d! It now has a health of %d!\n", player_attack, enemy_health);
        if (enemy_health <= 0) {
            puts("\nYou killed the enemy! Would you like to play again?");
            break;
        }

        int enemy_attack = rand() % 10;
        player_health = MAX(0, player_health - enemy_attack);
        printf("The enemy attacks! Your health went down by %d it is now %d\n\n", enemy_attack, player_health);
        if (player_health <= 0) {
            puts("You died. Would you like to play again to try to defeat the enemy?");
            break;
        }
    }

    return ask_number("[0 to exit, press anything else to play again]") != 0;
}


int main() {
    srand(time(NULL));

    if (ask_number("Do you wanna play this game? [Press 0 to exit or press any other number to play]") != 0) {
        do {
            puts("Okay\n");
            sleep_ms(300);
        } while (play());
    }

    puts("Goodbye");
    return 0;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.