TicTacToe with AI in ruby - follow-up overload

Question

I already posted this program numerous times, always surprised by the amount of improvement that is possible suggested by people here. Is it possible to make it even better, without nitpicking? At this point this is a special curiosity of mine =).

# win sequences constant
WIN_SEQUENCES = [[0, 1, 2], [3, 4, 5], [6, 7, 8],
 [0, 3, 6], [1, 4, 7], [2, 5, 8],
 [0, 4, 8], [2, 4, 6]]
# the game board
class Board
 attr_accessor :cells
 def initialize
 @cells = Array.new(9) # { nil }
 end
 def as_string
 stringed_board = @cells.map.with_index do |symbol, idx|
 symbol || idx + 1
 end
 stringed_board.each_slice(3).map do |row|
 " #{row.join(' | ')}"
 end.join("\n ---+---+---\n")
 end
 def cell_open?(position)
 @cells[position - 1].nil?
 end
 def win_game?(symbol)
 WIN_SEQUENCES.any? do |seq|
 return true if seq.all? { |a| @cells[a] == symbol }
 end
 end
 def full?
 @cells.all?
 end
 def place_symbol(position, symbol)
 @cells[position - 1] = symbol
 end
end
# game logic
class Game
 def initialize
 @board = Board.new
 # default states
 @player1 = Human.new(@board, 'Player 1', 'X')
 @player2 = AI.new(@board, 'Evil AI', 'O')
 welcome_msg
 start_screen
 end
 private
 def start_screen(choice = gets.chomp)
 until (1..3).include?(choice)
 exit if choice.downcase == 'exit'
 select_game_mode(choice.to_i)
 end
 end
 def select_game_mode(choice)
 case choice
 when 1 then [@player2 = Human.new(@board, 'Player 2', 'O')]
 when 3 then [@player1 = AI.new(@board, 'Kind AI', 'X'),
 @player2 = AI.new(@board, 'Evil AI', 'O')]
 else puts 'You silly goose, try again.'
 end
 @current_player = @player2
 display_board
 run_game
 end
 def welcome_msg
 print "\nWelcome to Tic Tac Toe.\n\n"
 puts 'Enter 1 to play against another player, 2 to play against an evil AI'\
 ', 3 to watch evil AI play against kind AI.'
 puts 'Type EXIT anytime to quit.'
 end
 def display_board
 puts "\e[H\e[2J" # ANSI clear
 print @board.as_string, "\n\n"
 end
 def run_game
 until game_over?
 swap_players
 check_and_place
 display_board
 display_result
 end
 end
 def game_over?
 @board.win_game?(@current_player.symbol) || @board.full?
 end
 def check_and_place
 position = @current_player.take_input
 @board.place_symbol(position.to_i, @current_player.symbol) unless position.nil?
 end
 def display_result
 if @board.win_game?(@current_player.symbol)
 puts "Game Over, #{@current_player.name} has won."
 exit
 elsif @board.full?
 puts 'Draw.'
 exit
 end
 end
 def swap_players
 case @current_player
 when @player1 then @current_player = @player2
 else @current_player = @player1
 end
 end
end
# human players in the game
class Human
 attr_reader :name, :symbol
 def initialize(board, name, symbol)
 @board = board
 @name = name
 @symbol = symbol
 end
 def take_input(input = nil)
 until (1..9).include?(input) && @board.cell_open?(input)
 puts "Choose a number (1-9) to place your mark #{name}."
 input = validate_input(gets.chomp)
 end
 input
 end
 private
 def validate_input(input)
 if input.to_i == 0
 exit if input.downcase == 'exit'
 puts 'You can\'t use a string, silly.'
 else
 position = validate_position(input.to_i)
 end
 position
 end
 def validate_position(position)
 if !(1..9).include? position
 puts 'This position does not exist, chief.'
 puts 'Try again or type EXIT to, well, exit.'
 elsif [email protected]_open? position
 puts 'Nice try but this cell is already taken.'
 puts 'Try again or type EXIT to, well, exit.'
 end
 position
 end
end
# AI players in the game
class AI
 attr_reader :name, :symbol, :board
 def initialize(board, name, symbol)
 @board = board
 @name = name
 @symbol = symbol
 end
 def take_input
 loading_simulation
 check_win
 return @finished if @finished
 check_block
 return @finished if @finished
 check_defaults
 return @finished if @finished
 # failsafe check
 (1..9).reverse_each { |i| return i if board.cells[i - 1].nil? }
 end
 private
 # first check if possible to win before human player.
 def check_win
 @finished = false
 1.upto(9) do |i|
 origin = board.cells[i - 1]
 board.cells[i - 1] = symbol if origin.nil?
 # put it there if AI can win that way.
 return @finished = i if board.win_game?(symbol)
 board.cells[i - 1] = origin
 end
 end
 def other_symbol
 case symbol
 when 'X' then 'O'
 else 'X'
 end
 end
 # if impossible to win before player,
 # check if possible to block player from winning.
 def check_block
 @finished = false
 1.upto(9) do |i|
 origin = board.cells[i - 1]
 board.cells[i - 1] = other_symbol if origin.nil?
 # put it there if player can win that way.
 return @finished = i if board.win_game?(other_symbol)
 board.cells[i - 1] = origin
 end
 end
 # if impossible to win nor block, default placement to center.
 # if occupied, choose randomly between corners or sides.
 def check_defaults
 @finished = false
 if board.cells[4].nil?
 @finished = 5
 else
 rand < 0.51 ? possible_sides : possible_corners
 end
 end
 def possible_sides
 [2, 4, 6, 8].each do |i|
 return @finished = i if board.cells[i - 1].nil?
 end
 end
 def possible_corners
 [1, 3, 7, 9].each do |i|
 return @finished = i if board.cells[i - 1].nil?
 end
 end
 def loading_simulation
 str = "\r#{name} is scheming"
 10.times do
 print str += '.'
 sleep(0.1)
 end
 end
end
Game.new

Answer 1

I'll try to analyze each class in your code separately. Board - update certain methods:

def as_string
 stringed_board = @cells.map.with_index(1) { |symbol, index| symbol || index }
#..... 
end
def cell_open?(position)
 !@cells[position - 1] # personal preference to use '!'
end
def win_game?(symbol)
 WIN_SEQUENCES.any? do |seq|
 seq.all? { |c| @cells[c] == symbol }
 # not necessary to return true explicitly
 end
end

Duplication in classes Human and AI(attr_readers and initialize method). I suggest to move it to parent class (call it Player):

class Player
 attr_reader :name, :symbol
 def initialize(board, name, symbol)
 @board, @name, @symbol = board, name, symbol
 end
end
class Human < Player
 # code without initialize method and attr_accessors
end
class AI < Player
 attr_reader :board
 # code ...
end

AI class:

class AI < Player
 attr_reader :board
 def take_input
 loading_simulation
 check_win_or_block(symbol) || check_win_or_block(other_symbol) || check_defaults
 # there was a lot of duplication here (with unnecessary explicit returns)
 end
 private
 def check_win_or_block(sym)
 # check_win and check_block have same logic and
 # can be replaced with one method
 finished = false
 0.upto(8) do |i|
 origin = board.cells[i]
 board.cells[i] = sym if !origin
 finished = i + 1 if board.win_game?(sym)
 board.cells[i] = origin
 end
 finished
 end
 def other_symbol
 symbol == 'X' ? 'O' : 'X' # I'm using ternary operator in such cases
 end
 def check_defaults
 # @finished variable is unnecessary now
 # possible_slides and possible_corners are pieces with the same logic
 # I replace them with possible_position that passes block
 if board.cells[4]
 rand < 0.51 ? possible_position(&:even?) : possible_position(&:odd?)
 else
 5
 end
 end
 def possible_position(&block)
 result = (1..9).select(&block).each do |i|
 return i + 1 if !board.cells[i]
 end
 result.is_a?(Integer) ? result : board.cells.rindex(nil) + 1
 # this line takes functionality of your code (take_input method):
 # (1..9).reverse_each { |i| return i if board.cells[i - 1].nil? }
 end
 def loading_simulation
 str = "\r#{name} is scheming"
 10.times { print str += '.'; sleep(0.1) }
 end
end

Class Game:

class Game
 def initialize
 # ..... 
 @player2 = AI.new(@board, 'Evil AI', 'O')
 @current_player = @player2
 # current_player is not a part of select_game_mode method functionality
 welcome_msg
 start_screen
 end
 private
 def start_screen(choice = gets)
 choice.strip!.downcase! if choice
 # In your code use until loop is not working. Reason:
 # 'gets' must be placed inside of this loop
 until %w(1 2 3 exit).include?(choice)
 # (1..3).include?(choice) in your code will always return false
 # because choice is a String
 puts 'You silly goose, try again.'
 start_screen(gets)
 end
 select_game_mode(choice)
 display_board
 run_game
 end
 def select_game_mode(choice)
 case choice
 when '1' then @player2 = Human.new(@board, 'Player 2', 'O')
 when '3' then @player1 = AI.new(@board, 'Kind AI', 'X')
 # @player2 = ... removed - it's already set.
 when 'exit' then exit
 # exit option must be here (I think).
 end
 end
 # ....
 def run_game
 until game_over?
 swap_players
 check_and_place
 display_board
 if game_over?
 puts display_result
 exit
 # exit moved from display_result. It changes state of program but not displaying result
 end
 end
 end
 # .....
 def display_result
 # It's "bad" style to place business logic and UI in one method.
 # code is not clear and harder to test
 # all puts's moved to run_game_method
 if @board.win_game?(@current_player.symbol)
 "Game Over, #{@current_player.name} has won."
 elsif @board.full?
 'Draw.'
 end
 end
 def swap_players
 @current_player = @current_player == @player1 ? @player2 : @player1
 # It's not essentially, but looks better.
 end
end

Overall, try not to mix business logic in your code with input/output functionality.

That's it for now. Will update my answer later.

• Glubi: BTW I think choice = gets.chomp.downcase is better then recursion in #start_screen
• Glubi: Your modifications for the first two methods seem to worsen the code readability without giving value back (e.g. performance). Player separation: nice catch! Didn't know you could do that with inheritance. AI class: I like your thinking here. I think you meant 0..8 instead of 1..8 in the default check. Game: nice, I missed it (select_game_mode was always executed anyway, oops). I wrote a comment before but was wrong about the finished variable.
• Sergii K: You need recursion in #start_screen. In your variant of this method if choise == '5' function exits from until-loop and start default game. Gets inside until-loop will ask user for input "until gets" expected result.
• Sergii K: About as_string and cell_open?. Difference in readability is unnoticeable. In #as_string method you've used with_index and it has 'original' possibility to pass starting index. Why not use it? How you estimate performance? I've write benchmark to compare original and modified methods. Results: (for as_string) * 1_000_000: user system total real updated 14.806 0.004 14.81 14.8432016 original 14.966 0.004 14.97 15.0057298 Results: (for cell_open?) * 100_000_000: original 18.524 0 18.524 18.5476748 updated 16.39 0.004 16.394 16.4081238
• Sergii K: "I think you meant 0..8 instead of 1..8 in the default check." - my typo. Method returns position (from 1 to 9), not index in @cells array. Must be: result = (1..9).select(&block).each do |i|