3
\$\begingroup\$

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
  attr_reader :name, :symbol

  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
  attr_reader :name, :symbol, :board

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

  def take_input
    loading_simulation
    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

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

Game.new
\$\endgroup\$
3
\$\begingroup\$

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
\$\endgroup\$
7
  • \$\begingroup\$ 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. \$\endgroup\$
    – Glubi
    Dec 18 '15 at 19:04
  • \$\begingroup\$ 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). \$\endgroup\$
    – Glubi
    Dec 23 '15 at 14:31
  • \$\begingroup\$ Using cases instead of if/else is slightly better performance-wise in ruby, just thought it was a good practice. \$\endgroup\$
    – Glubi
    Dec 23 '15 at 14:34
  • \$\begingroup\$ @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. \$\endgroup\$ Dec 29 '15 at 21:57
  • \$\begingroup\$ @Glubi The choice between a case statement and an if/else should never depend on performance IMHO. \$\endgroup\$ Dec 29 '15 at 21:58

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.