3
\$\begingroup\$

After getting familiar with C syntax by solving Project Euler Problems, I want to get something "useful" done for the first time.

A Mastermind clone seemed simple enough, and below you can check the current status. I know that some open issues are still pending (for example filtering the raw user input etc.), but at least it seems to work ok so far.

I only have FreeBSD on my Laptop, so I can't test if it works on Linux or with Cygwin, but I hope it does.

Any tips, suggestions, etc., are welcome.

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <termios.h>
#include <time.h>
#include <string.h>

#define VERSION "0.2"
#define MAXOPTIONS 4


struct termios saved_attributes;
void reset_input_mode(void);
void set_raw_mode(void);
void evaluate(char* set, int digits, int spots, int guesses, int timelimit);
void new_game(int digits, int spots, int guesses, int timelimit);
void custom_game(void);
void menu(void);


// Reset terminal to the state it had before
void reset_input_mode(void) {
    tcsetattr (STDIN_FILENO, TCSANOW, &saved_attributes);
}


// Set terminal to raw mode
void set_raw_mode(void) {
    struct termios tattr;
    char *name;

    if (!isatty (STDIN_FILENO)) {
        fprintf (stderr, "Not a terminal.\n");
        exit (EXIT_FAILURE);
    }

    tcgetattr (STDIN_FILENO, &saved_attributes);
    atexit (reset_input_mode);

    tcgetattr (STDIN_FILENO, &tattr);
    tattr.c_lflag &= ~(ICANON);
    tattr.c_cc[VMIN] = 1;
    tattr.c_cc[VTIME] = 0;
    tcsetattr (STDIN_FILENO, TCSAFLUSH, &tattr);
}


// Evaluate useres guess
void evaluate(char* set, int digits, int spots, int guesses, int timelimit) {
    char* guess = (char*)malloc(spots * sizeof(char));
    char* found = (char*)malloc(spots * sizeof(char));
    int correct = 0, semi;

    int guesscount = guesses == 0 ? 1 : 0;
    time_t start = time(NULL);

    while (correct < spots && guesses != guesscount) {
        if (timelimit > 0 && timelimit < (int)(time(NULL) - start)) {
            timelimit = 0;
        break;
                                                    }
        correct = 0;
        semi = 0;
        guesscount++;

        memcpy(found, set, sizeof(set));

        for (int i = 0; i < spots; i++) {
            read(STDIN_FILENO, &guess[i], 1);
            guess[i] -= '0';
            if (guess[i] == set[i]) {
                correct++;
                found[i] = 99;
            }
        }

        for (int i = 0; i < spots; i++)
            for (int j = 0; j < spots; j++)
                if (guess[i] == found[j]) {
                    found[j] = 99;
                    semi++;
                }

        printf(" | ");

        for (int i = 0; i < correct; i++)
            printf("○ ");
        for (int i = 0; i < semi; i++)
            printf("● ");

        printf("\n");
    }

    free(found);
    free(guess);
    
    if (correct == spots) {
        printf("Congratulations! You solved the puzzle in ");
        printf("%d seconds, ", (int)(time(NULL) - start));
        printf("using %d guesses.\n\n", guesses == 0 ? --guesscount : guesscount);
    }
    else if (timelimit == 0)
        printf("Sorry, time's up!\n\n");
    else
        printf("Sorry, you didn't solve the puzzle within %d guesses.\n\n", guesscount);
}


// Start a new game
void new_game(int digits, int spots, int guesses, int timelimit) {
    char* set = (char*)malloc(spots * sizeof(char));

    for (int i = 0; i < spots; i++) {
        set[i] = (1 + rand() % digits) % 10;
    }

    printf("Please enter your first guess:\n\n");
    set_raw_mode();
    evaluate(set, digits, spots, guesses, timelimit);
    free(set);
}


// Get parameters for a new game with custom settings
void custom_game(void) {
    int digits, spots, guesses, timelimit;

    printf("\nPlease enter the number of possible digits (2-10): ");
    scanf("%d", &digits);
    while (digits < 2 || digits > 10) {
        printf("Invalid number of digits, please enter a number from 2 to 10: ");
        scanf("%d", &digits);
    }

    printf("\nPlease enter the number of spots (2-10): ");
    scanf("%d", &spots);
    while (spots < 2 || spots > 10) {
        printf("Invalid number of spots, please enter a number from 2 to 10: ");
        scanf("%d", &spots);
    }

    printf("\nPlease enter the number of guesses (0 for unlimited): ");
    scanf("%d", &guesses);
    while (guesses < 0) {
        printf("Invalid number of guesses, please enter a positive number (or 0 for unlimited): ");
        scanf("%d", &guesses);
    }

    printf("\nPlease enter the time limit in seconds (0 for unlimited): ");
    scanf("%d", &timelimit);
    while (timelimit < 0) {
        printf("Invalid time limit, please enter a positive number (or 0 for unlimited): ");
        scanf("%d", &timelimit);
    }

    new_game(digits, spots, guesses, timelimit);
}


// Display the menu and get user's choice
void menu(void) {
    char c = 0;

    printf("+-----------------------------------------------------------------------------+\n");
    printf("|            Main menu - Enter your choice as a number from 1 to %d            |\n", MAXOPTIONS);
    printf("+-----------------------------------------------------------------------------+\n");
    printf("| 1 - New game in Classic mode (6 digits, 4 spots, 12 guesses, no time limit) |\n");
    printf("| 2 - New game in \"Super\" mode (8 digits, 5 spots, 12 guesses, no time limit) |\n");
    printf("| 3 - New game with custom settings                                           |\n");
    printf("| 4 - Quit (You can exit anytime by pressing Ctrl+C)                          |\n");
    printf("+-----------------------------------------------------------------------------+\n\n");
    printf("-> ");

    while(!c) {
        c = getchar();
        switch (c) {
            case '1': new_game(6, 4, 12, 0); break;
            case '2': new_game(8, 5, 12, 0); break;
            case '3': custom_game(); break;
            case '4': exit(0); break;
            default: {
                printf("Invalid option, please enter a number from 1 to %d\n-> ", MAXOPTIONS);
                while((c = getchar()) != '\n' && c != EOF);
                c = 0;
            }
            break;
        }
    }
}


int main(int argc, char **argv) {
    srand(time(NULL));
    printf("\nWelcome to Mastermind_C version %s\n\n", VERSION);
    menu();

    return EXIT_SUCCESS;
}
\$\endgroup\$
1
  • \$\begingroup\$ Minor: no real need for cast in timelimit < (int)(time(NULL) - start). If pedantic, use (time_t) timelimit < time(NULL) - start) \$\endgroup\$
    – chux
    Commented Jul 28 at 4:24

2 Answers 2

5
\$\begingroup\$

Check the return values of system and library calls:

// Reset terminal to the state it had before
void reset_input_mode(void) {
    tcsetattr (STDIN_FILENO, TCSANOW, &saved_attributes);
}

tcsetattr() returns -1 on failure. I'd appreciate it if this printed an error message to stderr on failure.


    tcgetattr (STDIN_FILENO, &saved_attributes);
    atexit (reset_input_mode);
    tcgetattr (STDIN_FILENO, &tattr);
    ...
    tcsetattr (STDIN_FILENO, TCSAFLUSH, &tattr);

Why continue if tcgetattr() fails? The atexit() function also returns a nonzero value on failure.


    char* guess = (char*)malloc(spots * sizeof(char));
    char* found = (char*)malloc(spots * sizeof(char));

malloc() and family returns a null pointer on failure. Failing to check for it risks invoking undefined behavior by a subsequent null pointer dereference.

Moreover, malloc() returns a generic void * that is implicitly converted to any other pointer type. As such, the cast is redundant and only serves to clutter the code.


read(STDIN_FILENO, &guess[i], 1);

This would return -1 on error too.

Do not use scanf() for user input:

scanf("%d", &spots);

The above line of code would invoke undefined behavior if the input exceeds the upper range of spots type. There's nothing you can do to prevent it. Moreover, it would consider 28ac as 28 and leave ac for the next read.

See: A beginners' guide away from scanf().

There's another problem too. scanf() returns the number of conversions successfully performed. You did not check whether the return value is 1.

I'd suggest reading a whole line with fgets() and then use strtol()/sscanf() to parse it.

getchar() returns an int, not a char:

From Linux's man page:

If the integer value returned by getchar() is stored into a variable of type char and then compared against the integer constant EOF, the comparison may never succeed, because sign- extension of a variable of type char on widening to integer is implementation-defined.

Consecutive string literals are concatenated:

So we can invoke printf() once for printing the menu:

#if 0
    printf("+-----------------------------------------------------------------------------+\n");
    printf("|            Main menu - Enter your choice as a number from 1 to %d            |\n", MAXOPTIONS);
    printf("+-----------------------------------------------------------------------------+\n");
    printf("| 1 - New game in Classic mode (6 digits, 4 spots, 12 guesses, no time limit) |\n");
    printf("| 2 - New game in \"Super\" mode (8 digits, 5 spots, 12 guesses, no time limit) |\n");
    printf("| 3 - New game with custom settings                                           |\n");
    printf("| 4 - Quit (You can exit anytime by pressing Ctrl+C)                          |\n");
    printf("+-----------------------------------------------------------------------------+\n\n");
    printf("-> ");
#else
printf("+-----------------------------------------------------------------------------+\n"
       "|            Main menu - Enter your choice as a number from 1 to %d            |\n"
       "+-----------------------------------------------------------------------------+\n"
       "| 1 - New game in Classic mode (6 digits, 4 spots, 12 guesses, no time limit) |\n"
       "| 2 - New game in \"Super\" mode (8 digits, 5 spots, 12 guesses, no time limit) |\n"
       "| 3 - New game with custom settings                                           |\n"
       "| 4 - Quit (You can exit anytime by pressing Ctrl+C)                          |\n"
       "+-----------------------------------------------------------------------------+\n\n"
       "-> ", MAXOPTIONS);
#endif

stdout and stderr may be connected to a terminal:

    if (!isatty (STDIN_FILENO)) {
        fprintf (stderr, "Not a terminal.\n");
        exit (EXIT_FAILURE);
    }

The error message should say that stdin is not connected to a terminal.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for taking the time! I'll look into the issues you pointed out. \$\endgroup\$
    – mattg
    Commented Jul 25 at 14:33
5
\$\begingroup\$

The code is clean and quite readable. This makes it highly suspicious and worthy of very close scrutiny...


Bug report:

void evaluate(char* set, ...
        ...
        memcpy(found, set, sizeof(set));

A common mistake is taking the sizeof a pointer thinking it is the count of elements in an array.
The "slush" of heap allocation of small quantities has prevented a SIGSEGV crash.
The remedy is super-simple, and left as an exercise.


Unnecessary text

The function prototypes at the top are probably unnecessary and should be removed.
Their presence, when main() is at the bottom, suggests there may be a mutual function call operation that is not present in this program.
KISS!


Should have had warning

void set_raw_mode(void) {
    struct termios tattr;
    char *name;

What is the unused name doing there? Exceedingly minor glitch (typo) of no consequence, but...

More of a concern is that the terminal enters raw mode and stays there even though i/o goes back to using both getchar() and scanf() for subsequent games. This may create some surprises for the user (eg: backspace doesn't work anymore??).

And, (I'm not going to test this) playing a second game that is offered would replace the original (cooked mode) terminal configuration, that has been saved, with the raw mode configuration that is still in effect from the first game played. atexit() is nice, but it's NOT going to achieve what the coder thinks it does. Oops!


More KISS'ing

int main(int argc, char **argv) { // #1
                                  // or
int main( void ) {                // #2

Which of the above is simpler for the reader to scan?

The compiler should have (again) warned you that two parameter variables are unused. The compiler is your friend and really wants to help you write the best code you can. Turn up its warning level and heed its critiques.

The compiler can really only validate the syntax of code, but, in this case, the warning strays into semantics. The presence of those two parameters implies they will have meaning to the function, but they don't! Is something not right in this function?? Consider this warning a bonus from the compiler!


Consistency

        exit (EXIT_FAILURE);
        ....
        exit (0);  // EXIT_SUCCESS???

As written, main() is too trivial, and evaluate() is where all the setup/teardown action happens. Suggest moving the guts of the latter into the former to have fewer functions to deal with. menu() simply interacts with the user, and returns a valid value (or, perhaps -1 to signify termination desired).

    char *name; // style 1
    ...
    char* found; // style 2

The placements of splats, in this code, isn't important... until it becomes important (i.e.: a bug arises.)
Recommend ALL splats be immediately adjacent to the variable name (style 1) and NOT joined to the datatype (style 2).


Don't do things just because you can

There's a ceiling of 10 "pegs" that can be played.
KISS, again!
Use all-or-part-of simple 10 byte arrays on the stack instead of complicating things with heap allocation/free invitations for bugs to arise.


Learning...

Here is a skeleton version of the same game.
Much can be gained from reading and understanding alternative approaches to solving the same problem.
With this challenge fresh in your mind, and having climbed-the-hill in your own way (good!), your brain is now fertile ground to learn and assimilate alternative techniques and practices.
Always try to enjoy what you do!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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