Посмотрите мою программу tic-tac-toe

Описание репозитория лучше сделать более информативным.

my version tic-tac-toe

Например, можно добавить, что игра написана на ruby. Можно дать ссылку на статью про игру в википедии.

Тут, у тебя, кстати, ошибка, пропущен предлог version OF tic-tac-toe.

files_txt — лучше назвать папку templates, во-первых, мало ли какие там файлы лежат, может быть, когда-нибудь не только текстовые будут. Во-вторых, название ничего не говорит о том, что это за файлы.

.gitignore — тут ряд странностей:

Заигнорирован, например, файл в папке files_txt, так лучше не делать. Если хочется куда-то складывать временные файлы, лучше создать (в т.ч. можно из программы) временную папку, например, tmp. Погугли, как из ruby создать папку, это несложно.

Заигнорены все *.md-файлы, меж тем, есть README.md, если ты его обновишь, то git не отследит изменений. Лучше так не делать. Убери эту строчку из .gitignore (как и про *.txt).

Все классы (в твоем случае файлы game.rb и screensaver.rb) лучше класть в папку lib, хоть мы этого и не говорили ;)

В файлах нужно аккуратно расставить отступы, в screensaver.rb сейчас ад. В руби принято делать отступы двумя пробелами, у тебя сейчас табуляции причем, как бог на душу положит. Выровняй все. Если надо, перенастрой ruby-mine или sublime.

Вот такие комментарии я называю капитанскими:

# Открываю файл
file = File.open(screen, "r:UTF-8")

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

Плюс тут несоответствие. Ридми у тебя на английском, а комментарии на русском. Выбери какой-то один язык для всего. Я бы взял русский, если с английским пока туго. Всё равно скорее всего, твои крестики-нолики англичанам пока не очень интересны :)

Если выровняешь отступы, то вот такие комментарии:

end# Финиш класса

Будут не нужны.

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

Как из руби создать папку. Ты имел в виду Dir.mkdir("/tmp/test") ?

Да, только проверь, что её ещё нет.

Dir.mkdir("tmp") unless File.exists?("tmp")

Я имел ввиду именно папку tmp, в неё уже вкладывать ничего не надо, туда прям файлы можно класть.

Кстати, никак пока не могу найти, где настроить отступы в рубимайне. ???

Нашёл!

Отлично! Жду правок в репозитории.

Я хочу еще искусственный интеллект прикрутить, как ты советовал, а потом все сразу и выложу.

Хорошо, но тогда пиши сразу с учетом моих замечаний: аккуратно размечая код и правильно называя переменные, файлы и папки ;)

Вы даете нереальные планы!:) Я попробую.)

Дело привычки + правильной настройки RubyMine/Sublime на самом деле.

Но это очень важно при устройстве на работу.

Скинул, теперь мазохистически жду порки.))

Отступы теперь супер! Читать стало удобнее и можно переходить к сути.

1. README.md в templates класть не надо, это я видимо неправильно сказал как-то. Ридми всегда в корне лежит. Тогда github его подхватит.

2. class Comp_ii классы в Ruby называются «кэмелкейсом»: без разделителей, каждое слово с большой буквы. Если предположить, что тут у тебя два слова, то вот так: class CompIi, но аббревиатура ИИ, это как два слова из одной буквы каждое, то есть вот так: class CompII.

3. def comp_ii(pl_moves) — название метода совпадает с названием класса и ничего не говорит про то, что это за метод. Я бы переименовал его в computer_move (если я правильно понял, то эта штука возвращает цифру, куда пойдет компьютер).

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

5. Я не очень понял, чем отличаются поля класса @three_move и @three_move2, почему не объединить их в один массив?

6. Все эти переменные (@four_move), кстати, не изменяются по ходу игры, они созданы, чтобы отражать текущую, в рамках реализации, логику ходов компа: их лучше сделать константами. Константы пишутся сплошь большими буквами в начале класса:

class CompII
  FOUR_MOVE = {
    ...
  }

  def some_method
    # тут доступна переменная FOUR_MOVE 
  end 

Пока так, правь, потом гляну снова.

@three_move от @three_move2 отличаются ходами. Например: если в @three_move [1, 5, 7] => 3 (комп ходит 3) А если тройка не срабатывает, подключается @three_move2 где [1, 5, 7] => 9 (комп ходит 9)

Переделал класс CompII и теперь на экран при первом ходе, вылезает вот такая ерунда:

Ход компьютера.
 __________________________Y
|        |        |        |
OO|   X    |        |        |
|        |        |        | 1
|________|________|________|
|        |        |        |
OO|        |        |        |
|        |        |        | 2
|________|________|________|
|        |        |        |
OO|        |        |        |
|        |        |        | 3
|________|________|________|
X   1         2        3

Сделайте ваш ход. (Как делать ход: x1 y1 и Enter)

Никак не пойму в чём тут дело?

        ONE_MOVE.each do |key, value|
          if key - pl_moves == []
            a = value.sample.to_i
            a
          end
        end

Вот этот код невозможно понять. Что такое key, что такое value, что такое a. Какую информацию несут эти переменные? Код должен сам за себя говорить, чтобы читающему не приходилось "компилировать" его в голове :)

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

Никак не пойму в чём тут дело?

Вряд ли класс comp_ii.rb виноват, он ведь не отвечает за вывод, а просто возвращает число.

Например: если в @three_move [1, 5, 7] => 3 (комп ходит 3) А если тройка не срабатывает, подключается @three_move2 где [1, 5, 7] => 9 (комп ходит 9)

Как это «тройка не срабатывает»?

Вряд ли класс comp_ii.rb виноват, он ведь не отвечает за вывод, а просто возвращает число.

А в выводе то вообще ничего не поменялось.

Как это «тройка не срабатывает»?

На месте тройки может быть ноль.

Переделал, теперь нормально?

Вряд ли класс comp_ii.rb виноват, он ведь не отвечает за вывод, а просто возвращает число.

Как я понял из теста и дебага, в момент от when до if меняется кодировка и pl_moves уже не читается как одна цифра.

На месте тройки может быть ноль.

А, комп не запоминает свои ходы?! :)

А почему не сделать этот функционал? Это было бы надёжнее гораздо.

Как я понял из теста и дебага, в момент от when до if меняется кодировка и pl_moves уже не читается как одна цифра.

Все разборки с кодировками надо делать один раз при чтении данных (из консоли, файла или откуда бы то ни было ещё).

Стало лучше, но теперь новые проблемы!

    case pl_moves
      when pl_moves.size == 1
        ...
      when pl_moves.size == 3
        ...
      when pl_moves.size == 3
        ...
      when pl_moves.size == 4
    end    

Во-первых, вспомни, как работает case, если у тебя всё зависит от pl_moves.size, то не надо его пихать в каждую ветку, достаточно в самом начале написать case pl_moves.size, а в when-ах писать только цифры.

Во-вторых, если pl_moves.size лежит тройка, то мы зайдем только в первую ветку, во вторую ветку с таким же условием мы никогда не зайдем, её с тем же успехом можно просто удалить.

В-третьих, в этой управляющей конструкции принято все when-ы ставить на том же уровне, что и сам case.

    case pl_moves.size
    when 1
      ...
    when 2
      ...
    when 3
      # тут вся логика для хода компа после 3-х ходов игрока
      # используй if-ы, если надо
    when 4
      # тут вся логика для хода компа после 4-х ходов игрока
      # используй if-ы, если надо
    end   

Снова жду правок!

Да у меня не было идеи, прямо таки грамотный ИИ делать, так, чтобы не совсем рандомно ходил и ладно. Просто там надо будет с ветвлениями заморачиваться, а я смысла особого в этом не вижу.

Меня больше волнует, какого рожна она работать перестала?

Обновил!

Опять добавил return-ы и всё заработало.

Да у меня не было идеи, прямо таки грамотный ИИ делать, так, чтобы не совсем рандомно ходил и ладно. Просто там надо будет с ветвлениями заморачиваться, а я смысла особого в этом не вижу.

Речь не о грамотности ИИ, а о грамотности автора программы.

Рассмотрим ситуацию с четырьмя ходами игрока:

    when 4
      FOUR_MOVE.each do |possible_pl_moves, comp_move|
        if possible_pl_moves - pl_moves == []
          return comp_move
        end
      end

      FOUR_MOVE2.each do |possible_pl_moves, comp_move|
        if possible_pl_moves - pl_moves == []
          return comp_move
        end
      end

В обоих массивах FOUR_MOVE и FOUR_MOVE2 есть например, ключ [1, 5, 7, 8]. Тогда если в первом цикле в какой-то момент выполнится условие possible_pl_moves - pl_moves == [], то мы вернем 3. И никогда не вернем девятку, т.е. до второго цикла дело никогда не дойдет. Даже, если на девятой позиции уже стоит ход компьютера.

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

Написал и проверку не сделал.(( Так всё работает: [https://paste2.org/e2CmIXda]

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

Как ни крути, без памяти о том, куда компьютер делал ходы, получается мутно.

Создай у экземпляра класса CompII поле @comp_moves и записывай в него ходы компа. Тогда у тебя все получится надежно и изящно.

Вадик, как мы сегодня case изменили, так он при запуске, на втором ходу стал глючить.

comp_move = TWO_MOVE[pl_moves]

Если первый ход сделать 5, а затем любую другую цифру, например 1 происходит глюк. Если в хеше у меня [1, 5] => 9 то он и ищет pl_moves = [1, 5] а на [5, 1] не реагирует. А с each ему очерёдность не важна.

Так работает:

TWO_MOVE.each do |possible_pl_moves, possible_comp_moves|
        if possible_pl_moves - pl_moves == []
          comp_move = possible_comp_moves
        end
      end

Сделай comp_move = TWO_MOVE[pl_moves.sort], а в массиваx типа TWO_MOVE всегда перечисляй ходы игрока в порядке возрастания номера клетки.

Как вариант.

Ну вроде всё!) Ссылка на гит в шапке обсуждения.

Переходим к game.rb

    # хеш всех ходов для проверки пользователя.
    @all_moves = {1 => "x1 y1", 2 => "x2 y1", 3 => "x3 y1", 4 => "x1 y2", 5 => "x2 y2",
                  6 => "x3 y2", 7 => "x1 y3", 8 => "x2 y3", 9 => "x3 y3"}
    # хеш с выигрышными комбинациями
    @positions_wins = {1 => [1, 2, 3], 2 => [4, 5, 6], 3 => [7, 8, 9], 4 => [1, 4, 7],
                       5 => [2, 5, 8], 6 => [3, 6, 9], 7 => [1, 5, 9], 8 => [3, 5, 7]}

Это тоже константы ведь? :)

p_f = current_path + '/../templates/playing_field.txt'

Не ленись, пиши названия переменных целиком, сейчас не очень понятно, что такое p_f.

@player_moves << move_player

Единообразнее называй переменные: по-английски move_player — двинуть игрока. А player_move — ход игрока.

    l2 = @all_lines_field[2].sub(move_player.to_s, "X")
    l6 = @all_lines_field[6].sub(move_player.to_s, "X")
    l10 = @all_lines_field[10].sub(move_player.to_s, "X")
    @all_lines_field[2] = l2
    @all_lines_field[6] = l6
    @all_lines_field[10] = l10

Вместо вот этого можно написать так:

@all_lines_field[2].sub!(move_player.to_s, "X")
@all_lines_field[6].sub!(move_player.to_s, "X")
@all_lines_field[10].sub!(move_player.to_s, "X")

Смысл вот этого метода не понял:

  def win(comp_or_player)
    # Прохожусь по выигрышным массивам
    @positions_wins.each_value do |win|
      # Если вместо атрибута метода будет выбран, массив ходов юзера
      # и если одно из значений(массив выигрышных комбинаций) минус
      # @player_moves будет равно []
      if comp_or_player == @player_moves &&
          win - comp_or_player == []
        puts "Вы выиграли!"
        abort

      elsif comp_or_player == @comp_moves &&
          win - comp_or_player == []
        puts "Выиграл компьютер."
        abort
      end
    end
  end

Если нужно просто выяснить, выиграл ли компьютер, зачем передавать такой сложный параметр. Либо сделай два метода

def computer_won?
  ...
end

def player_won?
  ...
end

Или один, который на вход принимает символ:

def win?(side)
  case side
  when :computer
    ...
  when :player
    ...
  end
end

Логику вывода результата на экран я бы выделил в отдельный метод.

Вноси правки пока, сам поищи возможность улучшить код на основе полученного опыта. Потом ещё посмотрим ;)

Если писать так:

@all_lines_field[2].sub!(move_player.to_s, "X")
@all_lines_field[6].sub!(move_player.to_s, "X")
@all_lines_field[10].sub!(move_player.to_s, "X")

Тогда получается, что вроде @all_lines_field переопределён, но на экран вывода не будет. А так:

l2 = @all_lines_field[2].sub(move_player.to_s, "X")
    l6 = @all_lines_field[6].sub(move_player.to_s, "X")
    l10 = @all_lines_field[10].sub(move_player.to_s, "X")
    @all_lines_field[2] = l2
    @all_lines_field[6] = l6
    @all_lines_field[10] = l10

вывод на экран есть.

Запушил изменения на гите.

Наконец, мы добрались до логики: у тебя сейчас два класса. Один называется ShowField (классы, кстати, обычно не называют с глаголов, можно просто Field), другой Game. Оба при этом периодически выводят поле на экран. Это не здорово.

Предлагаю тебе чётко разделить логику, так, чтобы Field занимался только выводом на экран, а Game — только игровой механикой.

Список ходов компьютера и игрока будет храниться в полях экземпляра Game, а все магические подстановки sub будут в методах Field.

Одно из полей Game хранит указатель на игровое поле, которое создается при создании новой игры.

Как-то так.

Чего-то наколдовал,так что теперь ИИ перестал работать, ходит только рандомно. Глянь, может ты поймёшь, а то до меня пока не доходит.

Дим, ни в коем случае нельзя скатываться в картину, когда ты не можешь разобраться в собственном коде и спрашиваешь меня. Используй дебагер, пиши тесты на отдельные методы, добейся локализации и понимания проблемы.

Вадик, спасибо что не подсказал! Я серьёзно, без всякой иронии. Так колоритно подебажил.) Нашёл пару дополнительных ошибок: Одна связана с ИИ, а вторая с основной частью проги. В общем всё сделал, запушил и прям собой доволен.))

Ну молодец!

  def player_moves
    @player_moves
  end

  def comp_moves
    @comp_moves
  end

Вот такие методы в ruby можно очень удобно записать одной строкой:

attr_reader :comp_moves, :player_moves

Пишется в начале класса, до конструктора, после констант (сработает везде, но просто так принято, попробуй).

end # Финал класса

Вот такие комменты лучше убирать. Что ещё может делать end в конце файла с определением класса? :)

  def comp_won?(comp_moves)
    POSITIONS_WINS.each do |win|
      if win - comp_moves == []
        puts "Выиграл компьютер."
        abort
      end
    end
  end

Метод с ? на конце возвращает true|false, abort и puts лучше делать уже в самой программе. Так будет нагляднее. Или сделать у Game метод game.result, чтобы можно было написать puts game.result if game.finished?.

Как реализовать метод game.finished?, думаю, догадаешься.

 def field
   ...
 end

Я не очень понял, что делает этот метод: по-моему, он читает шаблон поля из файла и выводит пустое поле на экран. Разбей на два метода: один читает из файла, а выводить будет следующий метод.

Потом у тебя есть ещё метод, который берет линии из массива и подставляет в них ход игрока (и отдельный метод для компьютера). Я бы совместил эти методы в один, который принимает на вход символ, который нужно поставить. Методы практически повторяют друг друга.

  while game.comp_moves.include?(comp_move) ||
      game.player_moves.include?(comp_move) do
    # комп будет искать вариант хода
    comp_move = ii.comp_move(game.player_moves)
  end

Вот эту проверку делать не нужно, у тебя comp_ii гарантирует, что компьютер пойдет правильно, вроде. Сейчас если даже комп и сходит случайно куда-то, куда он/игрок уже сходил, то он запомнит этот свой ход в локальном массиве и это не здорово, он будет считать, что ходит второй раз подряд.

Готово, запушил.

Дим, совершенно точно ты в файле show_move.rb повторил себя:

  def show_field(all_lines_field)
    ...
    line2 = field_all_lines[2].gsub(/[123]/, ' ')
    ...
  end

  def show_move(move, symbol)
    ...
    line2 = field_new[2].sub(move.to_s, symbol).gsub(/[123]/, ' ')
    ...
  end
end

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

В переменной @all_lines_field у тебя что? Шаблон поля, ведь? Переименуй её получше.

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

show_move.rb нужен для наглядности и удобства играющего, он выводит пустое поле один раз в начале игры. Получается, если объединить как ты предлагаешь в один метод, тогда пустое поле при каждом ходе, будет выводиться на экран, так-как попадёт в цикл.

Нет, я предлагаю написать метод, который будет выводить на экран поле с текущим набором ходов. И вызвать этот метод до начала игры (когда ходов нет). Тогда программа нарисует пустое поле, т.к. ходов ещё не было.

Понял, сделано! Запушил на гит.

Ну вот, теперь совсем огонь!

Форматирование ruby-кода ещё доведи до идеала:

https://github.com/bbatsov/ruby-style-guide

Вот полезный гем для этих целей от того же автора:

https://github.com/bbatsov/rubocop

Хм, думал форматировать будет проще чем оказалось. Столько всяких нюансов нашлось.

Запушил, что получилось, но возможно чего и пропустил.

Кра-со-та!

Кое-где мой глаз перфекциониста заметил, что ты не делаешь пустую строчку между методами

# комментарий к методу
def my_method
  ...
end
# комментарий к другому методу
def anothre_method
  ...
end

Рубокоп на это не ругается?

Ещё один момент заметил: у тебя в классе Game есть ассоциативный массив:

ALL_MOVES = {
  1 => 'x1 y1', 2 => 'x2 y1', 3 => 'x3 y1', 4 => 'x1 y2', 5 => 'x2 y2',
  6 => 'x3 y2', 7 => 'x1 y3', 8 => 'x2 y3', 9 => 'x3 y3'
}

А в самой программе есть вот такая штука:

  case player_move
  when 'x1 y1'
    player_move = 1
  when 'x2 y1'
    player_move = 2
  when 'x3 y1'
    player_move = 3
  when 'x1 y2'
    player_move = 4
  when 'x2 y2'
    player_move = 5
  when 'x3 y2'
    player_move = 6
  when 'x1 y3'
    player_move = 7
  when 'x2 y3'
    player_move = 8
  when 'x3 y3'
    player_move = 9
end

Попробуй вместо последнего написать так:

player_move = Game::ALL_MOVES.invert[player_input]

(Я бы все-таки сделал отдельную переменную для того, что ввел пользователь player_input, ты её сейчас перезаписываешь, можно запутаться)

Как проверишь, объясни, что это за конструкция и как мы так лихо сократили 20 строчек кода в 1! :)

Готово. По поводу конструкции, понял её так: Мы обращаемся к классу Game, в нем при помощи :: к константе ALL_MOVES и затем инвертируем ход игрока из строки в цифру.

Почти!

invert делает из массива ключ - значение массив значение - ключ :)

Да, именно это я и имел ввиду только описал коряво.:)