TicTacToe game with functional AI in ruby - follow-up

Question

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')
 @current_player = @player1
 start_screen
 end
 def start_screen
 @board.welcome_msg
 choice = nil
 until (1..3).include?(choice)
 choice = gets.chomp
 exit if choice.downcase == 'exit'
 game_modes(choice.to_i)
 end
 end
 def game_modes(choice)
 case choice
 when 1 then pvp_game
 when 2 then pva_game
 when 3 then ava_game
 else puts 'You silly goose, try again.'
 end
 end
 def pvp_game
 @board.display_board
 until game_over
 player_place_n_check
 swap_players
 end
 end
 def pva_game
 @board.display_board
 until game_over
 player_place_n_check
 swap_pva
 ai_place_n_check(@ai)
 swap_pva
 end
 end
 def ava_game
 @board.display_board
 until game_over
 ai_place_n_check(@ai2)
 swap_ava
 ai_place_n_check(@ai)
 swap_ava
 end
 end
 def game_over
 @board.win_game?(@current_player.symbol) || @board.full?
 end
 def player_place_n_check
 position = player_input
 @board.place_mark(position.to_i, @current_player.symbol)
 @board.display_board
 result?
 end
 def ai_place_n_check(player)
 position = player.ai_turn(@board)
 @board.place_mark(position.to_i, @current_player.symbol) unless position.nil?
 @board.display_board
 result?
 end
 def player_input
 input = nil
 until (1..9).include?(input) && @board.cell_open?(input)
 puts "Choose a number (1-9) to place your mark #{@current_player.name}."
 input = validate_input(gets.chomp)
 end
 input
 end
 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
 def 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
 def swap_pva
 case @current_player
 when @player1 then @current_player = @ai
 else @current_player = @player1
 end
 end
 def swap_ava
 @current_player == @ai ? @current_player = @ai2 : @current_player = @ai
 end
end
# AI player components
class AI
 attr_accessor :board, :name, :symbol
 def initialize(name, symbol)
 @name = name
 @symbol = symbol
 end
 def ai_turn(board)
 loading_simulation
 check_win(board)
 return @finished if @finished
 check_block(board)
 return @finished if @finished
 check_defaults(board)
 return @finished if @finished
 end
 # first check if possible to win before human player.
 def check_win(board)
 @finished = false
 1.upto(9) do |i|
 origin = board.board[i - 1]
 board.board[i - 1] = 'O' if origin.is_a? Fixnum
 # put it there if AI can win that way.
 return @finished = i if board.win_game?('O')
 board.board[i - 1] = origin
 end
 end
 # if impossible to win before player,
 # check if possible to block player from winning.
 def check_block(board)
 @finished = false
 1.upto(9) do |i|
 origin = board.board[i - 1]
 board.board[i - 1] = 'X' if origin.is_a? Fixnum
 # put it there if player can win that way.
 return @finished = i if board.win_game?('X')
 board.board[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(board)
 @finished = false
 if board.board[4].is_a? Fixnum
 @finished = 5
 else
 rand < 0.51 ? possible_sides(board) : possible_corners(board)
 end
 end
 def possible_sides(board)
 [2, 4, 6, 8].each do |i|
 return @finished = i if board.board[i - 1].is_a? Fixnum
 end
 end
 def possible_corners(board)
 [1, 3, 7, 9].each do |i|
 return @finished = i if board.board[i - 1].is_a? Fixnum
 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

Encapsulation

Inside the Game class:

attr_accessor :board, :player1, :player2, :ai, :current_player

Don't expose these attributes as they are only used inside the class. (In ruby you don't have to declare "instance variables / fields" to use them)

Also, make all internal methods private so they are not accessible from outside the class.

Single Responsibility Principle

Game should only be responsible for the logic of the game (having the players take turns). It should not be responsible for getting user input, so all user input related methods should be moved into Player (which I renamed to Human so it's obvious it only represents human players):

class Human
 attr_reader :name, :symbol
 def initialize(board, name, symbol)
 @board = board
 @name = name
 @symbol = symbol
 end
 def get_input
 ...
 puts "Choose a number (1-9) to place your mark #{@name}." # name is available directly
 ...
 end
 private
 def validate_input(input); ...; end
 def validate_position(position); ...; end
end

Note I added board to Human so it can validate the input. I renamed player_input to get_input (it's obvious it's the player's input now becuase it's inside Human). Now Game can use Human like this:

@player1 = Human.new(@board, 'Player 1', 'X')
position = @current_player.get_input

Polymorphism

A lot of code in Game is duplicated so it can handle both Human and AI. In order to avoid this duplication, we must be able to treat Humans and AIs the same way in the code. We can acheive this by making both classes expose the same interface.

The interface Human exposes is a single method called get_input which takes no arguments and returns a position. We can make AI expose this same interface by renaming ai_turn to get_input, and moving the board argument to the constructor (like we did in Human):

class AI
 attr_reader :name, :symbol
 def initialize(board, name, symbol)
 @board = board
 @name = name
 @symbol = symbol
 end
 def get_input # no arguments
 ...
 end
 private
 # no need to pass board around, simply use @board
 def check_win; ...; end
 def check_block; ...; end
 def check_defaults; ...; end;
 def possible_sides; ...; end;
 def possible_corners; ...; end;
 def loading_simulation; ...; end;
end

Now, if we have a variable called player, we can call player.get_input and it will work both in the case player is a Human and in the case it's an AI. We can treat both cases the same way.

This means Game can simply have 2 players, and the same logic will work for PvP, PvAI and AIvAI:

class Game
 def initialize
 @board = Board.new
 start_screen
 end
 private
 def start_screen
 @board.welcome_msg
 choice = nil
 until (1..3).include?(choice)
 choice = gets.chomp
 exit if choice.downcase == 'exit'
 game_modes(choice.to_i)
 end
 end
 def game_modes(choice)
 case choice
 when 1 then
 @player1 = Human.new(@board, 'Player 1', 'X')
 @player2 = Human.new(@board, 'Player 2', 'O')
 when 2 then
 @player1 = Human.new(@board, 'Player 1', 'X')
 @player2 = AI.new(@board, 'Evil AI', 'O')
 when 3 then
 @player1 = AI.new(@board, 'Evil AI', 'O')
 @player2 = AI.new(@board, 'Kind AI', 'X')
 else puts 'You silly goose, try again.'
 end
 @current_player = @player1
 run_game
 end
 def run_game
 @board.display_board
 until game_over
 player_place_n_check
 swap_players
 end
 end
 def game_over; ...; end
 def player_place_n_check
 position = @current_player.get_input
 @board.place_mark(position.to_i, @current_player.symbol) unless position.nil?
 @board.display_board
 result?
 end
 def result?; ...; end
 def swap_players; ...; end
end

Single Responsibility Principle (2)

Game still does too much. It handles the main menu which is not related to the game logic. It's best to move the code that shows the menu and constructs Humans/AIs outside the Game class.

This requires moving the board argument from the contructors of Human and AI to the get_input methods in both classes (sorry, didn't forsee it coming). Then make Game take the players as arguments so it can be instantiated like this (for example):

Game.new(Human.new('Player 1', 'X'), AI.new('Evil AI', 'O'))

---

Hopefully this fixes the structural problems with the code. After you follow these suggestions, you want to post a follow up question so you can get review for individual classes and methods.

Good luck, and keep learning!

• Glubi: That's awesome, I completely forgot about polymorphism. Everything you said is applied (except the last point because it's too much work I think :) ) Thanks!

Answer 2

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.

• Glubi: I improved the code for full board and win game as you suggested, thanks. @board uses numbers for user experience, which I prefer on readability (or needlessly writing more code). In any case, the Game class is big already, I'm not sure making it bigger is recommended (unless you have idea for another class separation?).
• Spike: @Glubi the numbers (as values of the array) are only needed when printing the board, but they hurt the readability of other code in Board as I've described. Game should indeed be split, I hope to write about this later.

Answer 3

First thing to catch my eye - by squinting and just looking at the "shape" of text on the screen - is the repetition in many methods:

• pvp_game vs pva_game vs ava_game in Game
• player_place_n_check vs ai_place_n_check in Game
• swap_players vs swap_pva in AI
• check_win vs check_block in AI

Obviously, they're not identical, but they certainly have more in common with each other than not.

This appears to stem from the way you've structured things. You do have a Player class, but it doesn't really do anything. It's just a name and a symbol. All the actions that a player might take are implemented in either Game or AI. The former seems out of place, while latter would make some sense, if AI inherited from Player - but it doesn't.

In the context of playing a game, there are always two players. Either of these might be human or AI. But what they have in common is that they're players.

So to me a simple class structure could be:

# Base class for all players
class Player; end
# More specific subclasses
class Human < Player; end
class Ai < Player; end

Both humans and AIs are players in this game; how they decide what move to make is what's different. But the game doesn't have to know or care about that. It should just tell the players to "make a move". That might involve asking a human with a prompt, or it might be an AI examining the board and deciding, but again: The game doesn't and shouldn't care about that, it just wants to know the move.

So I'd say that's the first step: Divide responsibilities appropriately.

• Glubi: Will do, thanks. Any more recommendation for a better code afterwards? Thanks.