Взгляд опытных программмистов, что улучшить и исправить?

Question

[Андрей]

Сделал игру крестики нолики. Хотел узнать от опытных программистов, что можно улучшить, исправить, или в каких моментах лучше сделать по другому.

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <locale.h>
#define SIZEFIELD 9
#define SPACE ' '
// field массив игрового поля 

// обнуление игрового поля
void reboot_field(char field[], const int size)
{
  int i;

  for (i = 0; i < size; i++){
    field[i] = SPACE;
  }
}

// Проверка игрового поля на победу или ничью 
int check_field( char field[])
{
  // проверка выигрыша первого игрока

  if(field[0] == 'X' && field[1] == 'X' && field[2] == 'X') {
    reboot_field(field, SIZEFIELD);
    return 1;
  }
  if( field[0] == 'X' && field[3] == 'X' && field[6] == 'X') {
    reboot_field(field, SIZEFIELD);

    return 1;
  }
  if ( field[0] == 'X' && field[4] == 'X' && field[8] == 'X' ) {
    reboot_field(field, SIZEFIELD);

    return 1;
  }
  if ( field[3] == 'X' && field[4] == 'X' && field[5] == 'X' ) {
    reboot_field(field, SIZEFIELD);

    return 1;
  }
  if ( field[6] == 'X' && field[7] == 'X' && field[8] == 'X') {
    reboot_field(field, SIZEFIELD);

    return 1;
  }
  if (field[6] == 'X' && field[4] == 'X' && field[2] == 'X') {
    reboot_field(field, SIZEFIELD);
    return 1;
  }
  if (field[1] == 'X' && field[4] == 'X' && field[7] == 'X') {
    reboot_field(field, SIZEFIELD);
    return 1;
  }

  // Проверка выигрыша второго игрока
  //
  if(field[0] == 'O' && field[1] == 'O' && field[2] == 'O') {
    reboot_field(field, SIZEFIELD);

    return 2;
  }
  if( field[0] == 'O' && field[3] == 'O' && field[6] == 'O') {
    reboot_field(field, SIZEFIELD);

    return 2;
  }
 if ( field[0] == 'O' && field[4] == 'O' && field[8] == 'O' ) {

    reboot_field(field, SIZEFIELD);
    return 2;
  }
  if ( field[3] == 'O' && field[4] == 'O' && field[5] == 'O' ) {

     reboot_field(field, SIZEFIELD);
    return 2;
  }
  if ( field[6] == 'O' && field[7] == 'O' && field[8] == 'O') {

    reboot_field(field, SIZEFIELD);
    return 2;
  }
  if (field[6] == 'O' && field[4] == 'O' && field[2] == 'O') {
    reboot_field(field, SIZEFIELD);
    return 2;
  }
  if (field[1] == 'O' && field[4] == 'O' && field[7] == 'O') {
    reboot_field(field, SIZEFIELD);
    return 2;
  }


  int cnt = 0, i;
  for(i = 0; i < SIZEFIELD; i++){
    if(field[i] != SPACE){
      cnt++;
    }
  }
  if (cnt == 9){

    reboot_field(field, SIZEFIELD);
    return 3;
  }


  return 0;
}


//  Печать игрового поля на экра
void print_field(const char field[], const int size, const char name[])
{
  system("clear");

/*  int i;

  puts("");
  for( i = 0; i < size; i++) {
    if( i == 3 || i == 6) {
      puts("");
    }
    printf("%c", field[i]);
    printf("%s", "___|");
  }
  puts("");
  
  */
  printf("\n\t\tХод игрока %s\n\n", name);
  printf("\n\t\t      |      |      \n");
  printf("\t\t   %c  |   %c  |   %c \n", field[0], field[1], field[2]);
  printf("\t\t______|______|______\n");
  printf("\t\t      |      |      \n");
  printf("\t\t   %c  |   %c  |   %c \n", field[3], field[4], field[5]);
  printf("\t\t______|______|______\n");
  printf("\t\t      |      |      \n");
  printf("\t\t   %c  |   %c  |   %c \n", field[6], field[7], field[8]);
  printf("\t\t      |      |      \n");

}

//  Режим игры против бота ( пока не реализован)
void game_with_computer(char field[], const int size)
{
  puts("Computer пока не умеет играть, но скоро мы его научим :)");
}


// Режим игры с человеком
void game_with_people(char field[], const int size){
  int player1, player2;
  int i;

  char name_player1[10];
  char name_player2[10];

  int check = 0;      // Проверка на победу

  system("clear");
  printf("\nВведите имя первого игрока ");
  scanf("%s", name_player1);
  system("clear");
  printf("\nВведите имя второго игрока ");
  scanf("%s", name_player2);

  while(check != 1 || check != 2 || check != 3) {
    print_field(field, size, name_player1);
    
    scanf("%d", &player1);
    if(player1 > 0 && player1 < 10) {
      field[player1 - 1] = 'X';
      check = check_field(field);
    }

    print_field(field, size, name_player2);

    if(check == 1) {
      printf("\nИгрок %s выиграл(а)!!!\n", name_player1);
      sleep(2);
      break;
    }

    if (check == 3) {
      printf("\nНичья\n");
      sleep(1.5);
      break;
    }

    scanf("%d", &player2);
    if(player2 > 0 && player2 < 10) {
      field[player2 - 1] = 'O';
      check = check_field(field);
    }

    if (check == 2) {
      printf("\nИгрок %s выиграл(а)!!!\n", name_player2);
      sleep(2);
      break;
    }

    if (check == 3) {
      printf("\nНичья\n");
      sleep(1.5);
      break;
    }
  }
}


int main(void)
{
  char *locale = setlocale(LC_ALL, "");
  void (*menu[3])(char*, int) = {game_with_computer, game_with_people};    // Меню

// Массив игрового поля
  char field[SIZEFIELD] = {SPACE, SPACE, SPACE, SPACE, SPACE, SPACE, SPACE, SPACE, SPACE};

  int choice;   // выбор пункта меню
  
  printf("\t\t\tКрестики нолики\n\n");
  puts("1) Играть с компьютером");
  puts("2) Играть с человеком");
  puts("3) Выход");

  printf ("( Для выбора пункта меню введите номер пункта) : ");
  scanf("%d", &choice);

  while(choice >= 0 && choice < 3) {
    (*menu[choice - 1])(field, SIZEFIELD);
    system("clear");

    printf("\t\t\tКрестики нолики\n\n");
    puts("1) Играть с компьютером");
    puts("2) Играть с человеком");
    puts("3) Выход");
    printf ("\n( Для выбора пункта меню введите номер пункта) : ");
    scanf("%d", &choice);
  }
  puts("Выход");

  return 0;
}

Comments

[Дмитрий]

Попробовать switch

Answer 1

[Дмитрий Беляев]

1. Много дублирующегося кода в функции check_field.
Можно вынести в константу массив массивов с выигрышными позициями и проверять в цикле.
При этом позиций проверяется 7, хотя их должно быть 8.

2. Проверку на ничью в той же check_field, можно делать ранний выход из цикла если найдена первая пустая клетка.

3. Вынести в enum результат функции check_field, сейчас там магические числа возвращаются.

4. Ввод имен игроков в функции game_with_people.
Буффер на 10 байт на стеке легко переполнить и будет UB.

5. В функции main опять повторяющийся код на вывод меню.

6. Нет проверки на некорректный ввод в scanf.

7. В game_with_people проверки на результат check_field можно сделать через switch case.

8. Если игрок введет некорректную клетку, то ход уйдёт другому игроку.

9. Можно перетереть уже занятую клетку.

Comments

[Андрей]

Спасибо, буду исправлять. Есть пару вопросов.

1. Предлагаешь вынести в константу массив массивов с выигрышами, я не совсем понял как это.

2. Проверка на ничью если найдена первая пустая клетка, я так пытался сделать, но получалось что при вводе первым игроком в любую ячейку, проверка запускалась и когда проверялся первый индекс если он был постым то игра заканчивалась сразу в ничью, хотя только был совершен первый ход.

[Дмитрий Беляев]

Андрей, по проверке на выигрыш:

#define WIN_COMBINATIONS_SIZE 8

const size_t win_combinations[WIN_COMBINATIONS_SIZE][3] = {
  {0, 1, 2},
  {0, 3, 6},
  // ...
};
const char checked_values[2] = {'X', 'O'};

for (size_t vi = 0; vi < 2; ++vi) {
  const char checked_value = checked_values[vi];
  for (size_t i = 0; i < WIN_COMBINATIONS_SIZE; ++i) {
    if (field[win_combinations[i][0] == checked_value &&
      field[win_combinations[i][1] == checked_value &&
      field[win_combinations[i][2] == checked_value)
    {
      reboot_field(field, SIZEFIELD);
      return vi + 1;
    }
  }
}

По поводу пустой клетки, сначала проверяем на победу, а потом уже на ничью:

for(size_t i = 0; i < SIZEFIELD; ++i){
    if(field[i] == SPACE){
      return 0;
    }
}

reboot_field(field, SIZEFIELD);
return 3;

[Андрей]

Дмитрий Беляев, спасибо большое, посмотрел на реализацию и сразу понятно стало, у самого пока мозгов не хватает чтобы более менее нормально делать:)

Answer 2

[Wataru]

1. #define SPACE ' ' - смысла не несет такая замена. Или какое-то более осмысленное имя, вроде EMPTY_FIELD, или просто в коде используйте ' '. Хотя лучше хорошее имя.

2. check_field - ужас. Во-первых, проверки на крестики и на нолики идентичны - только сравниваете вы или с 'X' или с 'O'. Ну напишите функцию, которая принимает char и ищет тройку из этих символов. Далее, используйте циклы. 3 строки можно проверить одним и тем же кодом, только индексы сдвигаются на 3 - вот и запустите цикл на 3 итерации. Так же со столбцами. В итоге вместо 8 условий будет только 4.
Вообще, можно до одного условия в трех вложенных циклах ужать:

const int dx[4] = {0, 1, 1, 1};
const int dy[4] = {1, 0, 1, -1};
for (dir = 0; dir < 4; ++dir) {
  for (int start = 0; start < 9; ++start) {
    int step;
    int x = start % 3;
    int y = start / 3;
    for (step = 0; step < 3; ++step) {
      if (x < 0 || x > 2 || y < 0 || y > 3 || field[x+3*y] != player) break;
      x += dx[dir];
      y += dy[dir];
    }
    if (step == 3) {
      return 1;
    }
  }
}
return 0;

3. print_field тоже делается циклами. Хотя бы по трем строкам.

4. основной игровой цикл можно делать while (1), ибо по любому из значений check происходит break;