# TicTacToe game with AI in ruby - follow-up

A week ago I posted my TicTacToe game follow-up question. The suggestions were referring mainly to the lack of polymorphism. Here's the new code, hopefully there's not a lot (or at all) to improve by now (except separating board functionality from Game class, but it seems to be too much of work anyway). As always, suggestions about structure, logic etc are welcome:

# 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)
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.any? do |seq|
return true if seq.all? { |a| @board[a] == symbol }
end
false
end

def full?
@board.any? 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
def initialize
@board = Board.new
start_screen
end

def start_screen(choice = nil)
@board.welcome_msg
@player1 = Human.new(@board, 'Player 1', 'X') # defaults
@player2 = AI.new(@board, 'Evil AI', 'O') # defaults
until (1..3).include?(choice)
choice = gets.chomp
exit if choice.downcase == 'exit'
game_modes(choice.to_i)
end
end

def game_modes(choice)
@board.display_board
case choice
when 1 then @player2 = Human.new(@board, 'Player 2', 'O')
when 3
@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
run_game
end

def run_game
until game_over
swap_players
check_and_place
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_mark(position.to_i, @current_player.symbol) unless position.nil?
@board.display_board
result?
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
end

# human players in the game
class Human

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 !@board.cell_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

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

def take_input
check_win(board)
return @finished if @finished
check_block(board)
return @finished if @finished
check_defaults(board)
return @finished if @finished
# failsafe check
(1..9).reverse_each { |i| return i if board.board[i - 1].is_a? Fixnum }
end

private

# 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

str = "\r#{name} is scheming"
10.times do
print str += '.'
sleep(0.1)
end
end
end

Game.new


# Board

Whenever I look at code, I first look at the shape and the color. When I look at the code for Board, I find a lot of mixed colors in my color scheme. This suggests to me that maybe you are mixing data with logic. There are also a lot literals in there. Perhaps you can extract these and replace them with named constants or methods?

Are you following the SRP? For instance, what does welcome_msg have to do with the board? Perhaps this is a little more ambiguous, but what about display_board?

For @board, you are using a 9-element array which seems okay. You might consider making it a 2d-array to make the public interface a little nicer, but I suppose it's fine. But why initialize them with the numbers 1 through 9? It seems to me that the board should be agnostic regarding it's contents. The indices already indicate the positions and having the contents be nil more clearly indicates that it is empty IMHO.

Are you happy with the argument name for #win_game? What about player or player_symbol? Later in the code you use the term mark, so what about mark? What about the method name? Is board supposed to know anything about the rules of the game?

The local variable sequences is really a constant. Consider extracting it. You might also want to break it up into rows, columns and diagonals:

ROWS = [[0, 1, 2], [3, 4, 5], [6, 7, 8]]
COLUMNS = [[0, 3, 6], [1, 4, 7], [2, 5, 8]]
DIAGONALS = [[0, 4, 8], [2, 4, 6]]


I always find that the literals true, false and nil are code smells in ruby. This is because ruby expressions are always implicitly truthy or falsey and either nil or not nil. This means that the expression return true if condition can almost always be written more succinctly and more efficiently as just condition (the exception being when you really need true and not just truthy).

# Game

I found this rather complex to read. A first suggestion would be to use attributes. That will get rid of all those @ signs :).

In #initialize you are calling start_screen. But start screen has nothing to do with initializing. It is already running the game. Why not move it to the run_game method?

Should all methods be public? What methods do you want clients to call?

You are setting up the player defaults in start_screen, only to then potentially change them later. Why not set them once and only once?

The method game_over is a predicate, so should be named game_over?. The method result? is not a predicate, so should be named result or perhaps something else like display_result.

I like the name of the method check_and_place, but should it be responsible for drawing the board and checking the result as well?

You might be better of using a plain old if-else instead of a case statement in swap_players

# Example code

Here are some mostly complete examples. I feel that there is more room to move stuff around, but they should indicate the things I touched upon. I feel that the Game example class still has to much conditions and to much raw data.

class Board
ROWS = [[0, 1, 2], [3, 4, 5], [6, 7, 8]]
COLUMNS = [[0, 3, 6], [1, 4, 7], [2, 5, 8]]
DIAGONALS = [[0, 4, 8], [2, 4, 6]]

def initialize
@cells = Array.new(9)
end

def [](position)
@cells[position - 1]
end

def []=(position, player)
fail RangeError unless (1..@cells.size).include? position
@cells[position - 1] = player
end

def full?
@cells.all?
end

def three_in_a_row?(player)
[ROWS, COLUMNS, DIAGONALS].any? do |sequence|
sequence.all? { |cell| cell == player }
end
end
end

class Game
def initialize
@board = Board.new
end

def run
welcome_message
run_game
end

private

def welcome_message
puts "\nWelcome to Tic Tac Toe.\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 run_game
p1, p2 = select_players

until game_over?
swap_current_player(p1, p2)
check_and_place
draw_board
end

display_result
end

def select_players
until (1..3).include?(choice = gets.chomp)
exit if choice.downcase == 'exit'
end

case choice
when 1 then [Human.new(@board, 'Player 1', 'X'), Human.new(@board, 'Player 2', 'O')]
when 2 then [Human.new(@board, 'Player 1', 'X'), AI.new(@board, 'Evil AI', 'O')]
when 3 then [AI.new(@board, 'Kind AI, 'X'), AI.new(@board, 'Evil AI, 'O')]
end
end

def game_over?
@board.three_in_a_row(@current_player) || @board.full?
end

def swap_current_player(p1, p2)
@current_player = (@current_player == p1 ? p2 : p1)
end

def display_result
if @board.three_in_a_row?(@current_player.symbol)
puts "Game Over, #{@current_player.name} has won."
else
puts 'Draw.'
end
end
end

• After all this time, wow! Thank you so much. I've since moved to focusing on Rails but I'll get to it as soon as possible. Thanks. Dec 18 '15 at 19:04
• Okay here's what I did: 1. I extracted the win sequences as a constant (tbh I did it some time after the initial post, it was glaring performance wise). 2. I moved display_board and welcome_msg to their rightful place at the Game(-logic) class. 3. I changed the array to nil, for some reason I thought it would require to refactor the whole code but actually I only needed to change all the is_a? to nil?, oops. 4. methods are now private accordingly and follow more strictly the naming conventions. 5. start_screen is just a cool trick I like (calling a method inside the initializer). Dec 23 '15 at 14:31
• Using cases instead of if/else is slightly better performance-wise in ruby, just thought it was a good practice. Dec 23 '15 at 14:34
• @Glubi It's always hard to "imagine" changes just from comments, so feel free to post new code. That said, I think you improved your code. Do you feel the same? 1. Constants are nice, really anything named is a good idea. 2. I think moving the methods to the Game class is an improvement. Don't be afraid to make new classes though. It's easy to put lots of responsibilities into a class named Game. 3. Be aware that you rarely need to explicitly check for nil. 4. Great! 5. A cool trick, sure, but you've coupled behaviour! Keep constructors as small as possible. Dec 29 '15 at 21:57
• @Glubi The choice between a case statement and an if/else should never depend on performance IMHO. Dec 29 '15 at 21:58