HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

TicTacToe with AI in ruby - follow-up overload

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withtictactoeoverloadfollowruby

Problem

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 k

Solution

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.

Code Snippets

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
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
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
  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

Context

StackExchange Code Review Q#114888, answer score: 4

Revisions (0)

No revisions yet.