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

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.

StackExchange Code Review Q#105996, answer score: 4