TicTacToe game with functional AI in ruby - follow-up

A month ago I posted an earlier version of the game here, and got a great response, which was mainly about the structure of my code. I freed some time today and reworked the code from scratch.

Things I improved:

• Class separation

• No more god object

• Applied single responsibility principle for classes and methods

I would love to hear if the code can be improved even further (structure, logic, etc.), as this is I feel the maximum of my coding capability:

```
# players in the game
class Player
attr_reader :name, :symbol

def initialize(name, symbol)
@name = name
@symbol = symbol
end
end

# the game board
class Board
attr_accessor :board

def initialize
@board = (1..9).to_a
end

def display_board
puts "\e[H\e[2J" # ANSI clear
@board.each_slice(3).with_index do |row, idx|
print " #{row.join(' | ')}\n"
puts ' ---+---+---' unless idx == 2
end
puts
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 cell_open?(position)
@board[position - 1].is_a?(Fixnum) ? true : false
end

def win_game?(symbol)
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]]

sequences.each do |seq|
return true if seq.all? { |a| @board[a] == symbol }
end
false
end

def full?
@board.each do |cell|
return false if cell.is_a? Fixnum
end
true
end

def place_mark(position, symbol)
@board[position - 1] = symbol
end
end

# game logic
class Game
attr_accessor :board, :player1, :player2, :ai, :current_player

def initialize
@board = Board.new
@player1 = Player.new('Player 1', 'X')
@player2 = Player.new('Player 2', 'O')
@ai = AI.new('Evil AI', 'O')
@ai2 = AI.new('Kind AI', 'X')
@cu

I will review the Board class.

def display_board
  puts "\e[H\e[2J" # ANSI clear
  @board.each_slice(3).with_index do |row, idx|
    print "  #{row.join(' | ')}\n"
    puts ' ---+---+---' unless idx == 2
  end
  puts
end

Board is responsible for the logic of the game board (keeping track of all cells and testing if a player has won). Therefore it should not be responsible for printing the board. It's better to return a string and have the Game class print it to screen.

Note that the loop is joining the rows, putting lines between them. It can be implemented with join. The method becomes: (after renaming)

def as_string
  @board.each_slice(3).with_index.map do |row, idx|
    "  #{row.join(' | ')}"
  end.join("\n ---+---+---\n")
end

I removed the code that clears the screen and adds an extra newline, because it is related to printing and not to the board logic. Instead, add a private method in Game which clears the screen and then prints @board.as_string and a newline.

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

This isn't related to the board logic either. This method belongs in Game, the only place where it is used. (Also rename it to print_welcome_msg)

def cell_open?(position)
  @board[position - 1].is_a?(Fixnum) ? true : false
end

is_a? returns a boolean, which you can return directly:

@board[position - 1].is_a?(Fixnum)

sequences.each do |seq|
  return true if seq.all? { |a| @board[a] == symbol }
end
false

You're checking whether any sequence in the array satisfies the condition. Use any?:

sequences.any? do |seq|
  seq.all? { |a| @board[a] == symbol }
end

Same for here:

def full?
  @board.each do |cell|
    return false if cell.is_a? Fixnum
  end
  true
end

def initialize
  @board = (1..9).to_a
end

def place_mark(position, symbol)
  @board[position - 1] = symbol
end

The @board array uses numbers for representing empty cells, which is very unintuitive. I would never guess that cell.is_a? Fixnum means "is the cell empty". Consider using nils:

def initialize
  @board = [nil] * 9
end

This changes cell.is_a? Fixnum to cell.nil?. Then, you need to change as_string so it uses the cell number for empty cells:

def as_string
  @board.map.with_index do |symbol, idx|
      symbol || idx + 1
  end.each_slice(3).with_index.map do |row, idx|
      "  #{row.join(' | ')}"
  end.join("\n ---+---+---\n")
end

(The only reason you used numbers is for printing the board, but numbers don't make sense as the contents of empty cells)

Finally, don't expose the board array:

attr_accessor :board

It should be a private attribute. You don't want users of the Board class to have acccess to any Array methods (e.g. push). Instead add to Board any needed methods, e.g.:

def [](idx)
    @board[idx]
end

def []=(idx, symbol)
    @board[idx]
end

Then you can use board[i] directly. (You may want to use idx+1 so Board provides a consistent interface which numbers the cells 1 to 9. And then maybe get rid of place_mark which will be the same as []=).

I would also rename board (the array) to cells.

StackExchange Code Review Q#108961, answer score: 2