# Simple TicTacToe game with AI in Ruby

What changes do you recommend from the perspective of structure, logic, etc?

# Checks if, for example, "2" is an integer "in disguise".
class String
def is_integer?
to_i.to_s == self
end
end

# Main TicTacToe game class
class Game
def initialize
@board = (1..9).to_a
@running = true
end

def display_board
puts "\n -----------"
@board.each_slice(3) do |row|
print '  '
puts row.join(' | ')
puts ' -----------'
end
puts
end

def determine_player(player)
if player == :X
return :X.to_s
elsif player == :O
return :O.to_s
end
end

def turn(chosen_player)
display_board
puts "Choose a number (1-9) to place your mark on, Player #{chosen_player}."
position = gets.chomp

# using personal created method to determine input
position = position.to_i if position.is_integer?

if @board.include?(position)
@board.map! do |num|
if num == position
determine_player(chosen_player)
else
num
end
end
elsif position.is_a?(String)
if position.downcase == 'exit'
puts 'Wow, rude. Bye.'
exit
end
puts 'Position can only be a number, silly.'
puts 'Try again or type EXIT to, well, exit.'
turn(chosen_player)
else
puts 'This position does not exist or already occupied, chief.'
puts 'Try again or type EXIT to, well, exit.'
turn(chosen_player)
end
end

def win_game?
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]]
b = @board

sequences.each do |sequence|
if sequence.all? { |a| b[a] == 'X' }
return true
elsif sequence.all? { |a| b[a] == 'O' }
return true
end
end
false
end

def draw?
@board.all? { |all| all.is_a? String } # returns true if no one has won.
end

def result?
if win_game?
display_board
puts 'Game Over'
@running = false
elsif draw?
display_board
puts 'Draw'
@running = false
end
end

def playergame_progress
while @running
turn(:X)
result?
break if !@running
turn(:O)
result?
end
end

# AI components
def try_sides
[1, 3, 5, 7].each do |idx|
return @board[idx] = 'O' if @board[idx].is_a? Fixnum
end
end

def try_corners
[0, 2, 6, 8].each do |idx|
return @board[idx] = 'O' if @board[idx].is_a? Fixnum
end
end

def ai_turn
# first check if possible to win before human player.
0.upto(8) do |i|
origin = @board[i]
@board[i] = 'O' if @board[i] != 'X'
win_game? ? return : @board[i] = origin # return for early breakout if won game.
end

# if impossible to win before player, check if possible to block player from winning.
0.upto(8) do |i|
origin = @board[i]
@board[i] = 'X' if @board[i] != 'O'
if win_game?
return @board[i] = 'O' # if player can win that way, place it there before him.
else
@board[i] = origin
end
end

# if impossible to win nor block, default placement to center.
# if occupied, choose randomly between corners or sides.
if @board[4].is_a? Fixnum
return @board[4] = 'O'
else
rand > 0.499 ? try_sides || try_corners : try_corners || try_sides
end
end

def thinking_simulation
str = "\rEvil AI is scheming"
5.times do
print str += '.'
sleep(0.3)
end
end

def aigame_progress
if rand > 0.3
while @running
turn(:X)
result?
break if !@running
thinking_simulation
ai_turn
result?
end
else
while @running
thinking_simulation
ai_turn
result?
break if !@running
turn(:X)
result?
end
end
end
end

def play
match = Game.new

puts 'Welcome to Tic Tac Toe'
puts 'Enter 1 to play against another player, or 2 to play against an evil AI.'
puts 'Type EXIT anytime to quit.'

choice = gets.chomp.to_i
case choice
when 1 then match.playergame_progress
when 2 then match.aigame_progress
else        puts 'You silly, you.'
end
end

play


Code above is case of fear of adding classes. This has two symptoms: Game being a god object, and over-attachment to primitives (or it's Ruby version, over-attachment to built-in classes). Interestingly, even in such simple example we can see why those are bad things.

Consider this two methods (I cut some code that is not important now):

def display_board
@board.each_slice(3) do |row|
puts row.join(' | ')
puts " ---------"
end
end

def draw?
@board.all? { |all| all.is_a? String } # returns true if no one has won.
end


Using Fixnums and String as elements of @board is admittedly smart and makes your code concise, but also quite unreadable, as evidenced by fact you needed a comment to explain method that is only single line long. Until someone (in particular: you, two months from now) reads your code thoroughly, it is not evident at first that is_a? String means an empty field (even with said comment). Similarly, it is not evident that row.join can produce something like 0 | X | 2. Consider example using a class instead of primitives. @board is now Array of Fields):

class Field
attr_accessor :owner

def taken?
owner.nil?
end

def empty?
!taken?
end
end

def draw?
@board.all &:taken?
end

def display_board
@board.each.with_index(1) |field, idx|
print "#{field.owner || idx} | "
print "\n-----------\n" if idx % 3 == 0
end
end


It's more verbose and still not ideal, as @owner is still a Symbol, but #draw? and #display_board now telegraph how they work much better (although #each_slice definitely looks better than a modulo, reader now doesn't need to scroll through code to find what #display_board actually prints. This would apply to other parts of code, i.e.

if @board.include?(position)


vs.

if @board[position].empty?


When using a class, we can describe what is being done, primitives force us to describe how things are done. Additional, possibly more important benefit is that methods like #draw? will work just the same even if we change how our Field class works - we achieved encapsulation.

God object is widely known code smell. Think about it: if you have single class, that is instantiated only once (and you do), you don't even need objects and classes - just change your @s to \$s, so that your state is held in global scope instead of object's (it is no different, since all your code uses the same state anyways!), and your code will work just the same. You probably know why globals are ill-advised, and as you can see your code works on globals, just pretends not to ;)

Instead of using single object that does everything, separate your logic in several classes and modules, following single responsibility principle. In your example, obvious candidate for extraction would be AI, and you seem to be partly aware of this, as evidenced by this comment:

 # AI components


Such extraction often requires you to pass around(as arguments) data that were previously just available, but makes code easier to read and refactor. For example, your AI class could have #player= method that allows to set it as :X or :O, and #next_move method that accepts current game state (a board, most likely) and returns number of field that AI takes in its move.

class AI
attr_accessor :player

def next_move(board)
# ... code code code ...
return 5 # or something else
# ... some more code ...
end
end


Extracting AI would allow you to modify your program easier, if (for example) you would want to allow spectating a match between two CPU players.

• Great stuff, I thought separating to classes is ill advised with code as simple as this, but I'll try applying what you've said. – Glubi Sep 30 '15 at 1:18
• @Glubi separating very simple code into classes/modules isn't very good thing to do, but your code isn't that simple - it ~200 lines long, and a common rule of thumb is that a class should be shorter than 100 lines, and many classes are even shorter, so a guess (without acctually analizing code) would be that your code can be divided at least in 3-4 parts reasonably. – Borsunho Sep 30 '15 at 6:24
• Hey, I completely reworked the code following your suggestions. Care to take a look and tell me if I did well? codereview.stackexchange.com/questions/108961/… – Glubi Oct 28 '15 at 2:57

determine_player can be simplified significantly:

def determine_player(player)
player.to_s
end


I have trouble understanding this:

return @board[idx] = 'O' if @board[idx].is_a? Fixnum


You are returning and assigning a value, while by the single responsability principle, you either return or set a value.

• Thank you, I've fixed the determine_player. I'm using return for both declaring and early breakout (otherwise it will keep iterating). – Glubi Sep 29 '15 at 16:05
• I could be wrong, but single responsibility principle isn't really about "single line of code should do single thing". – Borsunho Sep 29 '15 at 16:09
• @Borsunho correct, it is more often applied to methods, but I find it sensible to apply it to single lines too – Caridorc Sep 29 '15 at 16:17