5
\$\begingroup\$

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
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Glubi Sep 30 '15 at 1:18
  • \$\begingroup\$ @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. \$\endgroup\$ – Borsunho Sep 30 '15 at 6:24
  • \$\begingroup\$ 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/… \$\endgroup\$ – Glubi Oct 28 '15 at 2:57
0
\$\begingroup\$

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.

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.