7
\$\begingroup\$

This is my first full program I've written in C. It took me about 2 hours to pull together today. Looking for any sort of feedback on the code, formatting, cleanliness etc. I didn't follow any guides or tutorials, but did have some existing C++ knowledge and knowledge in other languages.

The game is Mastermind. The way it works is that the computer generates a 4 color code randomly from a pool of 7 colors. Then, the user must guess what the code is in as few guesses as possible. The user must guess 4 colors, and the feedback given is how many colors of the guess are a part of the code, and how many of the colors are placed correctly.

There's more on the Wikipedia page: https://en.m.wikipedia.org/wiki/Mastermind_(board_game)

Thanks

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <math.h>
#include <string.h>
#include <ctype.h>

#define RESET  "\x1B[0m"
#define BLOCK  "█"

#define RED   "\x1B[31m" BLOCK RESET
#define GRN   "\x1B[32m" BLOCK RESET
#define YEL   "\x1B[33m" BLOCK RESET
#define BLU   "\x1B[34m" BLOCK RESET
#define MAG   "\x1B[35m" BLOCK RESET
#define CYN   "\x1B[36m" BLOCK RESET
#define WHT   "\x1B[37m" BLOCK RESET

#define RED_C   "\x1B[31m"
#define GRN_C   "\x1B[32m"
#define YEL_C   "\x1B[33m"
#define BLU_C   "\x1B[34m"
#define MAG_C   "\x1B[35m"
#define CYN_C   "\x1B[36m"
#define WHT_C   "\x1B[37m"

#define COLORS 6
#define LENGTH 4

enum Colors {
    red,
    green,
    yellow,
    blue,
    magenta,
    cyan,
    white
};

static const enum Colors Color_map[] = {red, green, yellow, blue, magenta, cyan, white};

void *generate_colors(enum Colors *buffer)
{
    int power = pow(COLORS + 1, LENGTH);
    int colors_integer = rand() % power;

    for (int i = 0; i < LENGTH; ++i)
    {
        int remainder = colors_integer % (COLORS + 1);
        int divisor = colors_integer / (COLORS + 1);

        buffer[i] = Color_map[remainder];

        colors_integer = divisor;
    }
}

int convert_input(char *input, enum Colors *buffer)
{
    for (int c = 0; c < strlen(input); ++c)
    {
        char character = tolower(input[c]);

        switch (character)
        {
            case 'r':
                buffer[c] = red;
                break;

            case 'g':
                buffer[c] = green;
                break;

            case 'y':
                buffer[c] = yellow;
                break;

            case 'b':
                buffer[c] = blue;
                break;

            case 'm':
                buffer[c] = magenta;
                break;

            case 'c':
                buffer[c] = cyan;
                break;

            case 'w':
                buffer[c] = white;
                break;

            default:
                return 1;
        }
    }

    return 0;
}

char *color_to_char(enum Colors color)
{
    switch (color)
    {
        case red:
            return RED;
        case green:
            return GRN;
        case yellow:
            return YEL;
        case blue:
            return BLU;
        case magenta:
            return MAG;
        case cyan:
            return CYN;
        default:
            return WHT;
    }
}

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

    enum Colors selected_colors[4];
    generate_colors(selected_colors);

    int guessed = 0;

    do {
        char input[LENGTH];
        scanf("%s", input);

        if (strlen(input) == LENGTH)
        {
            enum Colors converted[LENGTH];
            int contains_unmatched = convert_input(input, converted);

            int correct_place = 0;
            int correct_color = 0;

            if (contains_unmatched)
            {
                printf("Please only choose characters from " RED_C "R, " GRN_C "G, " YEL_C "Y, " BLU_C "B, " MAG_C "M, " CYN_C "C, " WHT_C "W" RESET ".\n");
            }
            else
            {
                printf("You guessed: ");

                for (int i = 0; i < LENGTH; ++i)
                {
                    enum Colors color = converted[i];
                    enum Colors actual = selected_colors[i];

                    if (color == actual)
                        ++correct_place;

                    else 
                    {
                        for (int j = 0; j < LENGTH; ++j)
                        {
                            if (j != i)
                            {
                                enum Colors current = selected_colors[j];

                                if (color == current)
                                {
                                    ++correct_color;
                                    break;
                                }
                            }
                        }
                    }

                    printf("%s", color_to_char(converted[i]));
                }

                if (correct_place == LENGTH)
                {
                    printf("Well done! You got it right. Goodbye");
                    return 0;
                }
                else
                {
                    printf("\n %d correct color\n %d correct place and color\n", correct_color, correct_place);
                }
            }
        }
        else
        {
            printf("Please enter 4 characters.\n");
        }
    } while (!guessed);

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Can you add an explanation on what this is (supposed to be) doing? \$\endgroup\$ – yuri Mar 23 at 5:43
  • \$\begingroup\$ @yuri en.wikipedia.org/wiki/Mastermind_(board_game) and maybe also codereview.stackexchange.com/questions/202809/… \$\endgroup\$ – Oh My Goodness Mar 23 at 5:59
  • 1
    \$\begingroup\$ BTW, the program has a bug. Obviously noticeable with input like RRRR it can state: 2 correct color 2 correct place and color. How to I swap my R's to the correct order :-) If the hidden code only has two R it should report 0 correct color 2 correct place and color. \$\endgroup\$ – Bo R Mar 24 at 10:02
2
\$\begingroup\$
  • Maybe have a scheme to keep header files sorted. Makes it easier to spot possible duplicate includes.
  • enum Colors should be enum Color, every enum instance in only one at a time.

  • To keep track of a true enumerated sequence add max enum to keep track of the number of colors.

    magenta,
    cyan,
    white,
    
    COLORS }; // == 7
    

    (Thus, don't forget to remove #define COLORS 6.)

  • Color_map not really needed in this case since they are nicely enumerated. Just replace buffer[i] = Color_map[remainder]; with buffer[i] = (enum Color)remainder;.
  • Doesn't look like void *generate_colors(enum Colors *buffer) is returning anything, so just make it void generate_colors(....
  • Since COLORS now is 7 instead of 6, replace all COLORS + 1 with just COLORS.
  • In generate_colors, int power ... can be const int power ....
  • Interface to convert_input can be const for its input, i.e., int convert_input(const char *input, enum Color *buffer).
  • Simplify and speed up start of loop. E.g., don't check strlen(input) every interation, it's not changing. No need to call strlen (see below). tolower needs unsigned char to guarantee proper functionality, thus, platform dependent on signess of char.

Example

for (int c = 0;; ++c) // Look, no check for end of string done here
{
    switch (tolower((unsigned char)input[c]))
    {
        case 'r':

...

            case '\0': // add end-of-string check here
                return 0;
            default:
                return 1;
        }
    }
}
  • const char *color_to_char(enum Colors color) since it return const strings.
  • Input handling not kind on the user when entering wrong. Instead, read the whole line and parse the line for you guess.

Example

char* line = NULL;
size_t line_len = 0u;

while (!guessed && getline(&line, &line_len, stdin) != -1)
{
    char input[LENGTH + 1]; // OBS: This was too small before
    sscanf(line, " %4s", input); // automatic skip of white space
                                 // and limit input to 4 real characters
                                 // discarding the rest of the line.
  • Split wide printf statement and don't color the commas.

Snippet:

printf("%s\n", "Please only choose characters from "
RED_C "R" RESET ", "
GRN_C "G" RESET ", "
YEL_C "Y" RESET ", "
BLU_C "B" RESET ", "
MAG_C "M" RESET ", "
CYN_C "C" RESET ", "
WHT_C "W" RESET ".");
  • And if you use getline, free memory at end.

Snippet:

}

free(line);

return EXIT_SUCCESS;
\$\endgroup\$
  • \$\begingroup\$ Thank you, there's lots for me to consider here! I'm curious as to why it's necessary to free memory before exiting the program- is this not something the OS should do automatically? \$\endgroup\$ – JellyWX Mar 24 at 9:47
  • 1
    \$\begingroup\$ For me it's just customary, but is really help memory leak tools to help me identify the different types of leaks a program might have. By not free-ing it's harder to tell if if you have unintentional memory leaks that might eat up memory in the long run. But mostly, the OS will return the memory. \$\endgroup\$ – Bo R Mar 24 at 9:54
  • \$\begingroup\$ Okay, thanks. Another small query: I realize you replaced a do..while loop with a while loop. Is the only reason for this to do with the usage of getline in it, or are do..while loops something that should be generally avoided? \$\endgroup\$ – JellyWX Mar 24 at 12:03
  • 1
    \$\begingroup\$ Generally prefer while/for over do..while. They make it easier to understand the logic. In this case it was also to combine the termination of getline with your guessed logic. \$\endgroup\$ – Bo R Mar 24 at 13:51

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.