4
\$\begingroup\$

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 !@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

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

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!

\$\endgroup\$
  • \$\begingroup\$ 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! \$\endgroup\$ – Glubi Nov 6 '15 at 2:03
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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?). \$\endgroup\$ – Glubi Oct 29 '15 at 6:06
  • \$\begingroup\$ @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. \$\endgroup\$ – Spike Oct 29 '15 at 7:02
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Will do, thanks. Any more recommendation for a better code afterwards? Thanks. \$\endgroup\$ – Glubi Oct 29 '15 at 8:38

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.