Skip to main content
Code Review

Return to Answer

Demonstrate the typedef
Source Link
Toby Speight
  • 87.7k
  • 14
  • 104
  • 325

The typedef means that instead of writing out function arguments like

void print_board(const struct piece board[BOARD_HEIGHT][BOARD_WIDTH], int in_game);

they can be shorter and clearer:

void print_board(const game_board board, int in_game);

The typedef means that instead of writing out function arguments like

void print_board(const struct piece board[BOARD_HEIGHT][BOARD_WIDTH], int in_game);

they can be shorter and clearer:

void print_board(const game_board board, int in_game);
Source Link
Toby Speight
  • 87.7k
  • 14
  • 104
  • 325

One thing that strikes me immediately is that we pass struct piece board[8][8] around quite a lot. There's two things I would change there. The first is to use a typedef (or perhaps a small struct) to save all that typing. The other would be to give names to the dimensions.

#define BOARD_WIDTH 8
#define BOARD_HEIGHT 8
typedef struct piece game_board[BOARD_HEIGHT][BOARD_WIDTH];

If we want to build the game for a different height and width, then there will be only one place to change (once the loop bounds are adjusted to use the defined macros instead of literal 8). And we've made it easier if we ever want to transition to runtime-chosen board size in future (that's a more advanced exercise that you should try when you feel ready for it).


There's some weaknesses here:

 printf("\nEnter a row: ");
 row = getchar() - 48;
 getchar();
 printf("Enter a column: ");
 column = getchar() - 48;
 getchar();

Remember that any call to getchar() can return EOF. If that happens, we'll probably want to terminate the program, as we can't expect any useful input from that point onwards.

Secondly, the magic number 48 is specific to the character coding in use - we can make the program more portable by using '0' as is evidently intended.

It's also quite fragile - we're expecting a single character and newline, but users make mistakes and might enter multiple characters (and we'd probably want to allow that if we make the board bigger than 10✕10).

Thankfully, we have a better alternative to char-by-char input, and that's formatted input using scanf(). That would look something like

 printf("\nEnter a row and column: ");
 while (scanf("%hhd%hhd", &row, &column) != 2) {
 if (feof(stdin)) {
 /* scanf can never succeed now */
 fputs("Input failure\n", stderr);
 return EXIT_FAILURE;
 }
 /* Discard any remaining input */
 scanf("%*[^\n]"); /* ignore errors here */
 printf("Invalid input. Enter a row and column: ");
 }

That's not 100% robust, but it's an improvement.


A small fix: these function declarations should be prototypes, i.e. specify what arguments they take:

int generate_rand();
int main() {

The way we tell the compiler that they take no arguments at all (rather than unspecified arguments) is to write void between the parentheses:

int generate_rand(void);
int main(void) {

The structure is a bit unbalanced. There are a few low-level functions, but most of the logic is in a huge main(). Try dividing it into high-level functions - perhaps initialise_board() could be one of them?


There are more things that could be improved, but I hope this gives you some things to consider.

lang-c

AltStyle によって変換されたページ (->オリジナル) /