HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinorCanonical

TicTacToe game with AI in ruby - follow-up

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withtictactoegamefollowruby

Problem

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

Solution

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

Code Snippets

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]]
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

Context

StackExchange Code Review Q#110048, answer score: 3

Revisions (0)

No revisions yet.