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

Ultimate Tic-Tac-Toe in C

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

Problem

Here is my attempt at the UTTT code-challenge (in response to the the Weekend-Challenge Reboot). Here is what I would like critiqued:

-
I tested the code a few times for bugs, but I may have missed some.

-
I feel like I have duplicated code in some places (with only minor changes being the difference), refining them down a bit would be nice.

-
Better parsing of input

But any and all suggestions are acceptable. If you are interesting in looking at some occasionally updated versions of this code, take a look the the Github repository that houses the code (feel free to send fork and send pull requests).

`#include
#include
#include
#include

#define ROWS 9
#define COLS 9

typedef char Board[ROWS][COLS];
typedef char MetaBoard[ROWS / 3][COLS / 3];
typedef enum {VALID, NOT_A_DIGIT, NOT_IN_BOARD, SPACE_OCCUPIED, OUT_OF_BOUNDS} MoveStatus;

void fillSubBoard(Board board, int x, int y, char c)
{
for (; (x % 3) != 0; x--); // quickly set x to left bound of sub-board
for (; (y % 3) != 0; y--); // quickly set y to upper bound of sub-board
for (int rowMax = x + 2, row = x; row ROWS - 1 || column > COLS - 1) return NOT_IN_BOARD; // supplied coordinates aren't within the bounds of the board
else if (board[row][column] != '-') return SPACE_OCCUPIED; // supplied coordinates are occupied by another character
else if (rowBound == -1 && columnBound == -1) return VALID; // supplied coordinates can move anywhere
else if (((row > rowBound 3 + 2 || column > columnBound 3 + 2) ||
(row 0 && columnBound > 0)) return OUT_OF_BOUNDS; // coordinates aren't within the sub-board specified by the previous move
else return VALID; // didn't fail anywhere else, so coords are valid
}

int main(void)
{
int winner = 0, row = 0, column = 0, rowBound = -1, columnBound = -1, invalid = 0;
char tempRow = '\0', tempColumn = '\0';
Board board;
MetaBoard meta;
// initialize boards and fill with '-'
memset(board, '-', ROWS * COLS

Solution

A few comments:

[ ... ]

for (; (x % 3) != 0; x--); // quickly set x to left bound of sub-board
for (; (y % 3) != 0; y--); // quickly set y to upper bound of sub-board


I think I'd move the code to round to a multiple of three into a function of its own. I think I'd implement that something like this:

int round3(int in) { return (in/3)*3; }


[ ... ]

int getRowBound(int row)
{
    switch (row)
    {
        case 0 ... 2:
            return 0;
        case 3 ... 5:
            return 1;
        case 6 ... 8:
            return 2;
        default:
            return -1;
    }
}

int getColumnBound(int column)
{
    switch (column)
    {
        case 0 ... 2:
            return 0;
        case 3 ... 5:
            return 1;
        case 6 ... 8:
            return 2;
        default:
            return -1;
    }
}


These two functions (getRowBound and getColumnBound) are identical--and not just by coincidence either, so I think I'd merge them into a single function:

int getBound(int in) { 
    return (unsigned)in < 9 ? in / 3 : -1;
}


[ ... ]

for (; (row % 3) != 0; row--); // quickly set row to left bound of sub-board
for (; (column % 3) != 0; column--); // quickly set column to upper bound of sub-board


These should be calls to the round3 (or whatever name you prefer) mentioned previously.

[ ... ]

Although some disagree (vehemently in some cases) I'd personally prefer to get rid of some of the conditionals like these:

fillSubBoard(board, row, column, (player == 1) ? 'X' : 'O');
        meta[getRowBound(row)][getColumnBound(column)] = (player == 1) ? 'X' : 'O';


...and instead have something like:

static const char marks[] = {'X', 'O'};

// ...
fillSubBoard(board, row, column, marks[player]);
meta[getBound(row)][getBound(column)] = marks[player];


[ ... ]

MoveStatus validCoords(Board board, int row, int column, int rowBound, int columnBound)
{
    if (!isdigit((char)(((int)'0') + row)) && !isdigit((char)(((int)'0') + column))) return NOT_A_DIGIT; // supplied coordinates aren't digits 1-9


Any user input that you pass to isdigit (or any of the other isXXX functions/macros from ctype.h) should be cast to unsigned char first. Passing a negative number (other than EOF) to isXXX gives undefined behavior. In a typical case, any character outside the basic US-ASCII set (e.g., any letter that's not used in English, plus anything with a diacritic mark) will have a negative value when stored in a char.

[ ... ]

int winner = 0, row = 0, column = 0, rowBound = -1, columnBound = -1, invalid = 0;


Though some disagree, I think most programmers would prefer each variable in a separate definition. If (for some reason) you prefer not to do that, I'd at least format each variable onto a separate line.

for(; getchar() != '\n'; getchar()); // pick up superfluous input so we don't run into problems when we scan for input again


This looks buggy. In the condition you're calling getchar(), and checking the return value--but then in the increment part of the loop, you're calling getchar() again without checking the return value.

I think you probably want something more like:

while (getchar() != '\n')
    ;


[ ... ]

switch (validCoords(board, row, column, rowBound, columnBound))
        {
            case NOT_A_DIGIT:
                printf("Invalid input.  Re-enter: ");
                invalid = 1;
                break;
            case NOT_IN_BOARD:
                printf("Out of board's bounds. Re-enter: ");
                invalid = 2;
                break;
            case SPACE_OCCUPIED:
                printf("There is already an %c there.  Re-enter: ", board[row][column]);
                invalid = 3;
                break;
            case OUT_OF_BOUNDS:
                printf("Your move was in the wrong sub-board.  Re-enter: ");
                invalid = 4;
                break;


Here (again) I think I'd probably use the return value to index into an array:

static char const *errors[] = {
    "Invalid Input.",
    "Out of board's bounds",
    "That space is already used",
    "Your move was in the wrong sub-board"
};

int error;
while (0 != (error=validCoords(...))) {
    printf("%s Re-enter:", errors[error]);
    getinput();
}


This does require that you keep the list of errors in synch with the error numbers, but the payoff outweighs the extra burden (IMO).

[ ... ]

board[row][column] = (player == 1) ? 'X' : 'O';


Again, I'd use the marks mentioned earlier, so this would end up like:

board[row][column] = marks[player];

Code Snippets

for (; (x % 3) != 0; x--); // quickly set x to left bound of sub-board
for (; (y % 3) != 0; y--); // quickly set y to upper bound of sub-board
int round3(int in) { return (in/3)*3; }
int getRowBound(int row)
{
    switch (row)
    {
        case 0 ... 2:
            return 0;
        case 3 ... 5:
            return 1;
        case 6 ... 8:
            return 2;
        default:
            return -1;
    }
}

int getColumnBound(int column)
{
    switch (column)
    {
        case 0 ... 2:
            return 0;
        case 3 ... 5:
            return 1;
        case 6 ... 8:
            return 2;
        default:
            return -1;
    }
}
int getBound(int in) { 
    return (unsigned)in < 9 ? in / 3 : -1;
}
for (; (row % 3) != 0; row--); // quickly set row to left bound of sub-board
for (; (column % 3) != 0; column--); // quickly set column to upper bound of sub-board

Context

StackExchange Code Review Q#43466, answer score: 24

Revisions (0)

No revisions yet.