Возможные методы улучшения кода игры "крестики-нолики" на С#

Привел свой вариант кода ниже. По замечаниям:

Строгие замечания:

1. Имена локальных переменных метода должны называться с маленькой буквы по правилу lowerCamelCase. Чтобы узнать все правила именования в C#, рекомендую прочитать C# identifier naming rules and conventions

   Неправильно: bool WhoBool = true;

   Правильно: bool whoBool = true;

2. Используйте классы и ООП. C# является объектно-ориентированным языком программирования. Логику игры лучше вынести в отдельный класс. Сейчас же у вас половина логики игры находится в классе Program, а другая половина в FunctionsForGame.

3. В C# фигурные многострочные скобки настоятельно рекомендуется открываться с новой строки. Это касается как переменных так и методов, классов, пространств имен и т.д.

   Неправильно:

   string[,] Battlefield = {
       {" ", " ", " "},
       {" ", " ", " " },
       {" ", " ", " " },
   };
   

   Правильно:

   string[,] Battlefield =
   {
       {" ", " ", " " },
       {" ", " ", " " },
       {" ", " ", " " },
   };
   

4. Ваша игра никогда не закончится! цикл while(true) будет выполняться даже тогда, когда кто-то из игроков победил. Чтобы это исправить, достаточно выполнить break; при условии победы того или иного игрока

5. Нет вывода сообщения о ничье в случае, когда она наступит

6. Нет обработки ошибок. Если пользователь введет букву вместо координат, либо 1 координату вместо двух, то выскочит ошибка и приложение упадет. Мы же наверняка хотим, чтобы при неверном вводе пользователю высветилось сообщение, что введенные координаты не соответствуют требуемому формату.

7. int[] Coordinate = { Convert.ToInt32((Convert.ToString(CoordinateList[0]))),
   Convert.ToInt32((Convert.ToString(CoordinateList[1]))) };
   

   Зачем преобразовывать строку в строку? Можно же опустить преобразование:

   int[] Coordinate2 = 
   {
       Convert.ToInt32(CoordinateList[0]),
       Convert.ToInt32(CoordinateList[1])
   };
   

8. Не используйте dynamic, если можете обойтись без него! Вы не представляете, насколько сильно он может замедлить ваше приложение. Судя по коду, вы пытаетесь писать на C# как на языке с динамической типизацией (будто бы перешли с JavaScript). C# же строго типизированный язык программирования.

9. Метод Step нужно основательно переделать из-за его идеи возвращать результат как dynamic.

Нестрогие замечания:

1. bool WhoBool = true; Вместо явного указания типа при объявлении переменных можно использовать ключевое слово var: var whoBool = true;. Компилятор сам поймет, какой тип возвращает выражение справа. Явное указание типа переменной часто загромождает код, что часто может усложнять его чтение. Особенно это касается классов. Представьте, что у вас есть класс Person. Если явно писать тип, то получится Person person = new Person();. А ведь можно var person = new Person();.

2. Console.WriteLine("Приветствую в игре tic-tac-toe!\nВ простонародии крестики-нолики...\nЧтобы сходить, напиши координаты в виде \"1 2\"\n(1 - столбец, 2 - строчка)\nНажми enter, чтобы начать!");
   

   Нагляднее было бы написать инструкцию следующим образом:

       const string gameInstructions = """
   Приветствую в игре tic-tac-toe!,
   В простонародии крестики-нолики...,
   Чтобы сходить, напиши координаты в виде \"1 2\",
   (1 - столбец, 2 - строчка)
   Нажми enter, чтобы начать!
   """;
       Console.WriteLine(gameInstructions);
   

   Подробнее про литерал с тремя кавычками """ можно прочитать тут: raw string literal
3. Между методами класса рекомендуется оставлять 1 пустую строку
4. Для индикатора "Победа крестиков / Победа ноликов / Ничья / Игра еще не закончена" лучше использовать enum вместо числовых значений. Достаточно сложно постоянно держать в голове, какое число отвечает за какое состояние. Проще иметь именованные значения enum:

   enum GameState
   {
       NotOver,
       XWins,
       OWins,
       Draw,
   }
   

5. Метод CheckWin(), скорее всего, тоже можно куда проще и понятнее написать
6. Рекомендую основные методы размещать выше вспомогательных. Например, у вас метод Program.Main() находится ниже, чем Program.ProcessGame(). Таким образом можно будет читать код сверху вниз, последовательно
7. Как правильно заметил пользователь @CrazyElf, переменные, хранящие литералы, стоит выносить в отдельные константы. На этапе компиляции все упоминания именованных констант заменяются на значения этих констант, и таким образом мы избегаем выделения памяти под переменную.

Рефакторинг

Предложу свой вариант рефакторинга с исправлением всех тех замечаний, что указаны выше, а также:

1. Позволил себе метод Print(), который печатает игровое поле в консоль, вынести как отдельный extension метод, поскольку печать в консоль - это функционал, который должен находиться отдельно от класса игры крестики-нолики. Класс TicTacToeGame - это, грубо говоря, движок игры, а логика вывода на экран может отличаться в зависимости от того, какая технология используется для отображения игры.
2. Добавил различного рода проверки на неверный ввод координат используя кастомные исключения Exception. Таким образом мы можем отловить их и понять, в чем причина ошибки и вывести соответствующее сообщение пользователю

Структура файлов в проекте:

/Exceptions
    CellAlreadyFilledException.cs
    CoordinateOutOfBoundsException.cs
GameState.cs
Program.cs
TicTacToeGame.cs
TicTacToeGameConsoleExtensions.cs

Program.cs

using System;

class Program
{
    public static void Main(string[] args)
    {
        const string gameInstructions = """
Приветствую в игре tic-tac-toe!,
В простонародии крестики-нолики...,
Чтобы сходить, напиши координаты в виде \"1 2\",
(1 - столбец, 2 - строчка)
Нажми enter, чтобы начать!
""";
        Console.WriteLine(gameInstructions);
        Console.ReadLine();

        ProcessGame();
    }

    static void ProcessGame()
    {
        var game = new TicTacToeGame();

        var gameState = GameState.NotOver;
        while (gameState == GameState.NotOver)
        {
            game.Print();
            Console.WriteLine("Ожидание хода...");
            var userInput = Console.ReadLine();
            if (userInput == null)
            {
                continue;
            }

            var coordinateList = userInput.Split(" ");
            if (coordinateList.Length < 2
                || !int.TryParse(coordinateList[0], out var rowCoordinate)
                || !int.TryParse(coordinateList[1], out var columnCoordinate))
            {
                Console.WriteLine("Неверный формат координат. Повторите, пожалуйста, ввод.");
                continue;
            }

            try
            {
                game.MakeMove(rowCoordinate, columnCoordinate);
                gameState = game.GetGameState();
            } catch (CoordinateOutOfBoundsException)
            {
                Console.WriteLine("Вы вышли за пределы поля... Повторите, пожалуйста, ввод.");
            } catch (CellAlreadyFilledException ex)
            {
                Console.WriteLine($"Там уже есть \"{ex.CellValue}\", попробуй сходить в другое место...");
            }

        }

        game.Print();
        switch (gameState)
        {
            case GameState.XWins:
                Console.WriteLine("Победили \"X\"!");
                break;
            case GameState.OWins:
                Console.WriteLine("Победили \"O\"!");
                break;
            case GameState.Draw:
                Console.WriteLine("Ничья!");
                break;
        }
    }
}

TicTacToeGame.cs

public class TicTacToeGame
{
    private const string X = "x";
    private const string O = "o";

    private readonly string[,] _battlefield =
    {
        { " ", " ", " " },
        { " ", " ", " " },
        { " ", " ", " " },
    };
    private bool isXTurn = true;

    public string[,] Battlefield { get => _battlefield; }

    public TicTacToeGame()
    {
    }

    public void MakeMove(int rowCoordinate, int columnCoordinate)
    {
        var rowIndex = rowCoordinate - 1;
        var colIndex = columnCoordinate - 1;

        if (!AreIndexesInBounds(rowIndex, colIndex))
        {
            throw new CoordinateOutOfBoundsException();
        }

        if (IsCellFilled(rowIndex, colIndex))
        {
            throw new CellAlreadyFilledException(_battlefield[rowIndex, colIndex]);
        }

        
        _battlefield[rowIndex, colIndex] = isXTurn ? X : O;
        isXTurn = !isXTurn;
    }

    public GameState GetGameState()
    {
        int[,] winComb =
        { 
            { 0, 0,  0, 1,  0, 2 },
            { 1, 0,  1, 1,  1, 2 },
            { 2, 0,  2, 1,  2, 2 },
            { 0, 0,  1, 0,  2, 0 },
            { 0, 1,  1, 1,  2, 1 },
            { 0, 2,  1, 2,  2, 2 },
            { 0, 0,  1, 1,  2, 2 },
            { 0, 2,  1, 1,  2, 0 }
        };

        for (int i = 0; i < winComb.GetLength(0); i++)
        {
            if (_battlefield[winComb[i, 0], winComb[i, 1]] == _battlefield[winComb[i, 2], winComb[i, 3]]
                && _battlefield[winComb[i, 2], winComb[i, 3]] == _battlefield[winComb[i, 4], winComb[i, 5]]
                && _battlefield[winComb[i, 0], winComb[i, 1]] != " "
                && _battlefield[winComb[i, 2], winComb[i, 3]] != " "
                && _battlefield[winComb[i, 4], winComb[i, 5]] != " ")
            {
                var cell = _battlefield[winComb[i, 0], winComb[i, 1]];
                return cell == X ? GameState.XWins : GameState.OWins;
            }
        }

        return AreAllCellsFilled()
            ? GameState.Draw
            : GameState.NotOver;
    }

    private static bool AreIndexesInBounds(int rowIndex, int colIndex)
    {
        return rowIndex >= 0 && rowIndex < 3 && colIndex >= 0 && colIndex < 3;
    }

    private bool IsCellFilled(int rowIndex, int colIndex)
    {
        return _battlefield[rowIndex, colIndex] == X || _battlefield[rowIndex, colIndex] == O;
    }

    private bool AreAllCellsFilled()
    {
        for (var i = 0; i < _battlefield.GetLength(0); i++)
        {
            for (var j = 0; j < _battlefield.GetLength(1); j++)
            {
                if (!IsCellFilled(i, j))
                {
                    return false;
                }
            }
        }

        return true;
    }
}

TicTacToeGameConsoleExtensions.cs

public static class TicTacToeGameConsoleExtensions
{
    public static void Print(this TicTacToeGame game)
    {
        var battlefield = game.Battlefield;

        for (int i = 0; i < battlefield.GetLength(0); i++)
        {
            string SeparationColumn = "\n-----------";
            for (int j = 0; j < battlefield.GetLength(1); j++)
            {
                string SeparationRow = " |";
                if (j == battlefield.GetLength(1) - 1)
                {
                    SeparationRow = "";
                }
                Console.Write($" {battlefield[i, j]}{SeparationRow}");
            }
            if (i == battlefield.GetLength(0) - 1)
            {
                SeparationColumn = "";
            }
            Console.WriteLine(SeparationColumn);
        }
    }
}

GameState.cs

public enum GameState
{
    NotOver,
    XWins,
    OWins,
    Draw,
}

CellAlreadyFilledException.cs

public class CellAlreadyFilledException : Exception
{
    public string CellValue { get; }
    public CellAlreadyFilledException(string cellValue) : base()
    {
        CellValue = cellValue;
    }
}

CoordinateOutOfBoundsException.cs

public class CoordinateOutOfBoundsException : Exception
{
}

То, что я сейчас напишу не будет являться ответом на конкретно поставленный вопрос, ибо я не буду разбирать ваш код и ошибки вашего кода (это было сделано мной в комментариях, а также в соседнем ответе). Данным ответом я покажу как ваш код должен выглядеть, ибо в данной теме что-то все на одинаковые грабли наступают...

И так, C# является объектно-ориентированным языком, а это в свою очередь значит, что мы должны писать код, следуя ООП, то есть разбивать все на логические объекты, которые мы сможем легко изменить/заменить/добавить в будущем.

Разобравшись с этим, давайте теперь зададим вопрос:

Какие объекты есть в игре "Крестики-Нолики"?

В данной игре я вижу как минимум 3 объекта, а именно:

• Игрок - Этот объект описывает игрока (его символ, история ходов, и др., что относится именно к игроку). Почему это отдельный объект? А потому, что игроков может быть 2, 3, 10, и так далее, все ведь зависит от правил, которые продумает автор конкретной игры.
• Поле - Данный объект описывает игровое поле, его размеры, его методы изменения на поле, и т.д.
• Игра - Собственно, основой объект, который всем руководит и отвечает за запуск/остановку игры.

К этому списку можно добавить еще объекты (таблица лидеров например), но это уже будет зависеть от конкретной реализации игры. В данном случае нам достаточно этих 3-х.

Теперь

Давайте реализуем это все

• Игрок - Тут все просто, это просто класс, который будет содержать в себе символ. Почему класс? Как я и писал выше, это сделано для того, чтобы была возможность в будущем, без каких либо проблем, расширить код любым функционалом.

  public class Player(char symbol)
  {
      public char Symbol { get; } = symbol;
  }
  

• Поле - Вот что такое поле? Это некая сетка, содержащая в каждой ячейке конкретный символ. То есть класс поля должен содержать в себе эту сетку, методы по ее заполнению и редактированию, а также методы проверки сетки (проверка победы). Получим тогда такое:

  public class Board
  {
      private readonly char[,] _board;
      public int Size { get; }
  
      public Board(int size)
      {
          Size = size;
          _board = new char[Size, Size];
          Initialize();
      }
  
      private void Initialize()
      {
          for (var row = 0; row < Size; row++)
              for (var col = 0; col < Size; col++)
                  _board[row, col] = '.';
      }
  
      public char[,] GetBoard()
      {
          return (char[,])_board.Clone();
      }
  
      public bool IsValidMove(int row, int col)
      {
          return row >= 0 && row < Size && col >= 0 && col < Size && _board[row, col] == '.';
      }
  
      public void MakeMove(int row, int col, char player)
      {
          _board[row, col] = player;
      }
  
      public bool CheckWin(int row, int col)
      {
          var currentPlayer = _board[row, col];
  
          return CheckLine(Enumerable.Range(0, Size).Select(i => _board[row, i]), currentPlayer) ||
                 CheckLine(Enumerable.Range(0, Size).Select(i => _board[i, col]), currentPlayer) ||
                 (row == col && CheckLine(Enumerable.Range(0, Size).Select(i => _board[i, i]), currentPlayer)) ||
                 (row + col == Size - 1 && CheckLine(Enumerable.Range(0, Size).Select(i => _board[i, Size - i - 1]), currentPlayer));
      }
  
      private bool CheckLine(IEnumerable<char> line, char player)
      {
          return line.All(cell => cell == player);
      }
  }
  

  Код может показаться сложным из-за LINQ, понимаю, но поверьте, там нет ничего сложного.

  • readonly char[,] _board - Это собственно сетка поля, которая содержит конкретный символ игрока, либо нейтральный символ.

  • public int Size { get; } - Размер сетки. Обратите внимание, это публичное свойство, которое имеет только get, то есть данное значение мы можем получить где угодно, но не можем его изменить.

  • public Board(int size) - Конструктор класса, через который мы задаем размер поля, ну и затем инициализируем все необходимое.

  • Initialize() - У нас есть сетка, но она пустая, игрок не увидит ничего, если если мы захотим вывести ее на экран. Собственно для этого нам надо заполнить каждую ячейку поля, неким нейтральным символом. В моем случае это точка.

  • GetBoard() - Нам надо получить все поле целиком для его дальнейшего вывода. Я лично возвращаю копию, но по сути, этого можно и не делать (зависит от задач).

  • IsValidMove - Проверяем, может-ли игрок поставить в указанную ячейку свой символ. То есть мы смотрим, заданный столбец и строка в пределах сетки (больше или равно 0 и меньше размера), а также смотрим на то, какой символ в данной ячейке, свободна-ли она (точка - свободная клетка).

  • MakeMove - Просто устанавливаем в указанную ячейку нужный символ. Этот метод можно объединить с проверкой, возвращая bool значение (сделан ход или нет), тут уж смотрите сами как вам удобней.

  • CheckLine - Это вспомогательный метод, который получает коллекцию символов и конкретный символ, ну и проверяет через LINQ, все-ли символы в коллекции являются теми, что мы ищем, если все, то возвращаем true, иначе false. При помощи данного метода будет легко понять, выиграл-ли игрок или нет.

  • CheckWin - А это собственно и проверка на выигрыш. Тут немного запутанно все, но ничего сложного. Вот что такое выигрыш в крестиках-ноликах? Выигрыш - это когда все символы в одной линии одинаковы (конкретного игрока). Линия может быть горизонтальная, вертикальная, ну и две диагональных. Собственно все эти линии мы и проверяем.

    • При помощи Enumerable.Range(0, Size) мы генерируем числовой список, который начинается от 0 и имеет в себе Size значений. То есть, если будет Size = 3, то мы получим коллекцию, которая будет в себе иметь [0, 1, 2]. Другими словами мы взяли так все индексы конкретной строки/столбца.

    • .Select(i => _board[row, i]) - Имея индексы строк, нам надо забрать их символы, собственно через Select (LINQ) мы это и делаем, забирая в указанной строке все символы.

    • .Select(i => _board[i, col]) - Аналогично, но уже для столбцов.

    • .Select(i => _board[i, i]) - Аналогично, но для диагональной линии, где мы забираем 0х0 символ, 1х1, 2х2, и так до конца.

    • .Select(i => _board[i, Size - i - 1]) - Противоположная диагональ (0x2, 1x1, 2x0).

    • Как только мы забрали нужные символы, мы отправляем их CheckLine методу, который сверяет с указанным, ну и если одна из проверок даст true (в данной строке все символы одного из игроков), то игрок выиграл (возвращаем true).

    • Метод не совсем эффективный, можно и лучше, но для небольшого проекта, думаю в самый раз.

• Игра - В этом классе прописываем основную логику самой игры, ее запуск, ее ходы, взаимодействие с пользователем, и так далее. Получим что-то такое:

  public class Game
  {
      private readonly Board _board;
      private readonly List<Player> _players;
      private readonly IGameRenderer _renderer;
      private int _turn;
  
      public Game(int size, List<Player> players, IGameRenderer renderer)
      {
          if (size < 3)
              throw new ArgumentException("Размер доски должен быть не менее 3.");
  
          if (players == null || players.Count < 2)
              throw new ArgumentException("Должно быть как минимум два игрока.");
  
          _board = new Board(size);
          _players = players;
          _renderer = renderer;
      }
  
      public void Play()
      {
          var isGameRunning = true;
  
          while (isGameRunning)
          {
              _renderer.RenderBoard(_board.GetBoard());
              var currentPlayer = _players[_turn % _players.Count];
              _renderer.DisplayMessage($"Ход игрока {currentPlayer.Symbol}");
  
              (int Row, int Col) move;
              do
              {
                  move = _renderer.GetMove();
              } while (!_board.IsValidMove(move.Row, move.Col));
  
              _board.MakeMove(move.Row, move.Col, currentPlayer.Symbol);
              _turn++;
  
              if (_board.CheckWin(move.Row, move.Col))
              {
                  _renderer.RenderBoard(_board.GetBoard());
                  _renderer.DisplayMessage($"Игрок {currentPlayer.Symbol} выиграл!");
                  isGameRunning = false;
              }
              else if (_turn == _board.Size * _board.Size)
              {
                  _renderer.RenderBoard(_board.GetBoard());
                  _renderer.DisplayMessage("Это ничья!");
                  isGameRunning = false;
              }
          }
      }
  }
  

  • Сначала у нас идет инициализация всех данных, как видите, мы создаем объект поля, список игроков, текущий ход, а также IGameRenderer (о нем расскажу позже).

  • Ну а дальше, я не знаю, смысла объяснять наверно нету. Просто идет бесконечный цикл, пока игра не закончится. Каждую итерацию цикла выводим поле на экран, спрашиваем игрока куда поставить его символ, делаем ход, проверяем победу или ходы (сетка 3х3 = 9 ходов, если за 9 ходов побед нет, то это ничья), ну и так все повторяем.

• Ну и стартовая точка (консоль) - Просто инициализируем класс игры с нужными данными и запускаем:

  static void Main(string[] args)
  {
      Console.Write("Введите размер доски (3 по умолчанию): ");
      var size = int.TryParse(Console.ReadLine(), out var parsedSize) ? parsedSize : 3;
  
      Console.Write("Введите количество игроков (2 по умолчанию): ");
      var playerCount = int.TryParse(Console.ReadLine(), out var parsedPlayerCount) ? parsedPlayerCount : 2;
  
      var players = new List<Player>();
      for (var i = 0; i < playerCount; i++)
      {
          Console.Write($"Введите символ для игрока {i + 1}: ");
          var symbol = Console.ReadLine()?.FirstOrDefault() ?? '?';
          players.Add(new Player(symbol));
      }
  
      var game = new Game(size, players, new ConsoleRenderer());
      game.Play();
  }
  

Вот собственно и весь проект игры. Ах да, IGameRenderer... Им я хотел показать, что ваш код должен быть максимально гибким. Вот допустим сейчас вы пишете эту игру для консоли, а если потом нужен будет полноценный интерфейс? А может сетевое взаимодействие? А может через файл будете взаимодействовать... То есть понимаете, что консоль - это некая изменяемая часть, которую желательно отделять от общей логики проекта. Вот собственно IGameRenderer это и делает.

IGameRenderer - это интерфейс, некий "контракт", которому должен обязательно следовать конкретный класс. Благодаря этому, мы можем написать логику, но реализовать ее потом. Давайте взглянем на это все:

public interface IGameRenderer
{
    void RenderBoard(char[,] board);
    void DisplayMessage(string message);
    (int Row, int Col) GetMove();
}

Как видите, в данном интерфейсе я прописал требуемые моему коду методы, а именно вывод поля, вывести сообщение, а также запросить у игрока ходы. Эти все методы я запрашиваю где мне надо, например _renderer.DisplayMessage($"Ход игрока {currentPlayer.Symbol}");.

Но без реализации это ничего работать не будет. Давайте сделаем реализацию данного интерфейса для консоли:

public class ConsoleRenderer : IGameRenderer
{
    public void RenderBoard(char[,] board)
    {
        Console.Clear();
        int size = board.GetLength(0);
        for (int i = 0; i < size; i++)
        {
            for (int j = 0; j < size; j++)
            {
                Console.Write(board[i, j] + " ");
            }
            Console.WriteLine();
        }
    }

    public void DisplayMessage(string message)
    {
        Console.WriteLine(message);
    }

    public (int Row, int Col) GetMove()
    {
        Console.Write("Введите строку: ");
        var row = (int.TryParse(Console.ReadLine(), out var parsedRow) ? parsedRow : 0) - 1;

        Console.Write("Введите столбец: ");
        var col = (int.TryParse(Console.ReadLine(), out var parsedCol) ? parsedCol : 0) - 1;

        return (row, col);
    }
}

Как видите, это все тот-же объект (класс), который наследуется от IGameRenderer и реализует все его методы. Аналогичные классы мы можем создать под любой нужный нам вид взаимодействия с пользователем, достаточно будет подставить нужный при инициализации игры (var game = new Game(size, players, new ConsoleRenderer());).

Результат всего этого:

Победа:

Собственно, основной посыл - делите все на объекты, методы, классы, интерфейсы. Это поможет вам в дальнейшем изменять, расширять, улучшать свой проект без особых проблем. Удачи!