patterncMajor
Ultimate Tic-Tac-Toe in C
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
-
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:
[ ... ]
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:
[ ... ]
These two functions (
[ ... ]
These should be calls to the
[ ... ]
Although some disagree (vehemently in some cases) I'd personally prefer to get rid of some of the conditionals like these:
...and instead have something like:
[ ... ]
Any user input that you pass to
[ ... ]
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.
This looks buggy. In the condition you're calling
I think you probably want something more like:
[ ... ]
Here (again) I think I'd probably use the return value to index into an array:
This does require that you keep the list of errors in synch with the error numbers, but the payoff outweighs the extra burden (IMO).
[ ... ]
Again, I'd use the
[ ... ]
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-boardI 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-boardThese 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-9Any 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 againThis 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-boardint 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-boardContext
StackExchange Code Review Q#43466, answer score: 24
Revisions (0)
No revisions yet.