Like Tree1Likes
  • 1 Post By grumpy

Need Review for a Program I just made.

This is a discussion on Need Review for a Program I just made. within the C Programming forums, part of the General Programming Boards category; Hello everyone! This is my first time posting on this forum, and I'd like for some people to review my ...

  1. #1
    Registered User
    Join Date
    Jun 2014
    Posts
    1

    Need Review for a Program I just made.

    Hello everyone! This is my first time posting on this forum, and I'd like for some people to review my code for a small game I made. It's a typical Random Number Guessing Game, and I was wondering how I could improve this code. I am still new to C, although not new to programming. I started with Python.

    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <time.h>
    
    
    int main(void) {
        int running = 1;
        while (running = 1) {
            char tryAgain = 'n';
            int random;
            static int userIn;
            int counter = 0;
            srand(time(NULL));
            random = rand();
            random = random % 100;
            printf("Welcome to Random Gems!\n");
            printf("Your computer has already randomly generated a number between 1 and 100.\n");
            printf("It is your job to find what number that was in (7) tries.\n");
            for (int i = 0; i < 7; i++) {
                printf("Your guess: ");
                scanf("%d", &userIn);
                if (userIn > random) {
                    printf("Too high, try again.\n");
                }
                else if (userIn < random) {
                    printf("Too low, try again.\n");
                }
                else if (userIn == random) {
                    printf("Congratulations! You got it in %d tries!\n", counter);
                    break;
                }
                else {
                    printf("INPUT error! EXITING program!");
                    break;
                }
                counter++;
            }
            printf("Try again?[y/n]: ");
            scanf("%s", &tryAgain);
            if (tryAgain == 'y') {
                printf("Restarting the game!");
            }
            else if (tryAgain == 'n') {
                break;
            }
            else {
                printf("INPUT error! EXITING program!");
                break;
            }
        }
        printf("Thank you for playing!");
        getch();
        return 0;
    }

  2. #2
    Registered User
    Join Date
    May 2010
    Posts
    2,919
    Look at the following snippet:
    Code:
        while (running = 1) {
    In C/C++ the operator= is the assignment operator, and operator== is the comparison operator. When you use the assignment operator in your while loop you have an infinite loop, which is equilavent to while(1).

    Also the srand() function should be called only once.

    And you need to study up on the scanf() format specifiers, the "%s" specifier is for a C-string, not a single character, tryAgain is a single character.

    Lastly getch() is a non-standard function that you should avoid. You should really prefer the getchar() function instead. And if you must use getch() you need to #include the proper include file for this non-standard function.

    Jim

  3. #3
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    22,313
    Well...
    • You declared the variable named running, but do not use it other than to assign 1 to it in the condition of the while loop.
    • You seeded the PRNG by calling srand within the loop, which means it happens on every iteration. Rather, this should be done only once, usually near the start of the main function.
    • There does not appear to be any good reason for userIn to be declared static. If you want it to retain its value between iterations, declare it before the loop. However, since it obtains its value from the scanf call, it could well be declared non-static within the loop and it will still work.
    • Instead of writing:
      Code:
      random = rand();
      random = random % 100;
      write:
      Code:
      random = rand() % 100;
    • Avoid magic numbers. You want to limit the user to 7 tries, to declare a named constant with the value of 7, then use it for both the printf and to control the for loop.
    • Check the return value of scanf to determine if the user entered invalid input. Your if-else chain checks if userIn is greater than, less than or equal to the random number... thus the final else clause will never be reached.
    • The %s format specification for scanf is meant for reading a string into an array. If you do not specify the field width, it makes you vulnerable to buffer overflow. In this case, you are trying to use it to read into a char, which is wrong. Use %c to read a char.
    • The getch function is non-standard. You can run your program from within a separate command prompt window.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  4. #4
    Registered User Alpo's Avatar
    Join Date
    Apr 2014
    Posts
    561
    I couldn't really find improvements to the code that other's haven't caught, but I did notice this line:

    Code:
     printf("Try again?[y/n]: ");
            scanf("%s", &tryAgain);
            if (tryAgain == 'y') {
                printf("Restarting the game!");
            }
    In order to implement this properly, a good method might be to have the user choice to play the game in a while loop that surrounds the game (which I would move to its own function). It's always a good idea to break unique behaviors into their own functions, it might not matter much in a small game like this, but it is vital once you get larger code going.

    I wrote a little game in the same manner as the one you have (number guessing) to show you what I mean:

    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <time.h>
    #include <ctype.h>
    #define MAXGUESS 7
    #define MAXRAND 201
    
    void guessing_game(void);
    int compare_guess(int,int);
    void nlkill(void);
    
    int main()
    {
        srand(time(NULL));
        char playchoice = 'b';
        int numbergames = 0;
    
        while(playchoice != 'n'){
            printf("Would you like to play %s?\n\n",
                   (numbergames > 0) ? "again" : "");
            printf("[Y] / [N] >");
    
            if(scanf(" %c", &playchoice) != 1){
                printf("Please enter numbers only\n\n");
            }
    
            playchoice = tolower(playchoice);
    
            if(playchoice == 'y'){
                guessing_game();
                ++numbergames;
            }
    
            if(playchoice != 'n' || playchoice != 'y'){
                nlkill();
            }
        }
    
        printf("Thank U 4 being such a great playa!\n\n");
    
        return 0;
    }
    
    void nlkill(void)
    {
        while(getchar() != '\n');
    }
    
    void guessing_game(void)
    {
        int random;
        int guesses = 0;
        int playerguess;
        random = rand()%MAXRAND+1;
    
        while(guesses <= MAXGUESS){
    
            printf("\nGuess a number between 1 and %d\n\n>", MAXRAND);
    
            if(scanf(" %d", &playerguess) != 1){
                printf("Please enter numbers only\n");
            }
    
            if(compare_guess(playerguess, random) == 1){
                printf("Congratulations, you're a big wiener!\n\n");
                return;
            }
    
            else ++guesses;
        }
    
        printf("You a LOSER!\n\n");
    }
    
    int compare_guess(int player, int random)
    {
        if(player < random){
            printf("\nThat's the lowest of the low.\n\n");
            return 0;
        }
    
        else if(player > random){
            printf("\nYou are so high right now!\n\n");
            return 0;
        }
    
        return 1;
    }

    In the main function you have the outer loop, which asks if the player wants to play. If the player is on their 2nd game or later, a ternary operator will add "again" to "Do you want to play?". You have a bit of checking the return of scanf, and also putting the player choice variable through a function called tolower() which will make it lowercase (so we can compare it). If input is invalid, I flush the input buffer with nlkill() (getchar !='\n').

    Then the guessing_game() function is the body of the actual game, it's in its own function so that I can easily see where it is, and know what behaviors it should display. This function then uses the compare function to determine if the player has won.

    On a final note, one thing you might want to think about (not really a coding thing), is the conceptual side of design. For instance, in a game like this, if the player uses the most efficient manner of guessing, is there a statistical likelihood they could still lose (would you want them too?). As it is, using a simple binary search, the player is almost certainly going to win.

  5. #5
    Registered User
    Join Date
    Jun 2011
    Posts
    76
    Why is userIn static ?

  6. #6
    Registered User
    Join Date
    Jun 2005
    Posts
    6,679
    Quote Originally Posted by Alpo View Post
    <snip>but I did notice this line:

    Code:
     printf("Try again?[y/n]: ");
            scanf("%s", &tryAgain);
            if (tryAgain == 'y') {
                printf("Restarting the game!");
            }
    In order to implement this properly, a good method might be to have the user choice to play the game in a while loop that surrounds the game (which I would move to its own function). It's always a good idea to break unique behaviors into their own functions, it might not matter much in a small game like this, but it is vital once you get larger code going.
    One rather significant problem with the code Alpo quoted is that tryAgain is a single char, and %s can potentially read to multiple chars. If the user enters almost anything other than whitespace this code exhibits undefined behaviour - which is a problem as this code is being used to read a yes/no type responses.
    vart likes this.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Sunshine, and read this, this, and this before posting again.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. program review
    By melestorm in forum C++ Programming
    Replies: 1
    Last Post: 11-12-2009, 05:30 AM
  2. Program review
    By MikeyVeeDubb in forum C Programming
    Replies: 1
    Last Post: 09-24-2009, 02:29 PM
  3. program review
    By shuaib.akram in forum C Programming
    Replies: 3
    Last Post: 08-17-2007, 12:59 AM
  4. Review my Program
    By Sentral in forum C++ Programming
    Replies: 3
    Last Post: 07-27-2005, 06:07 PM
  5. Please Review my Program
    By Unregistered in forum C Programming
    Replies: 2
    Last Post: 04-20-2002, 08:19 PM

Tags for this Thread


1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21