Return to Revisions

Not bad.

Unbounded recursion

Inside the Game class:

def initialize
  # ...
  play_game
end

def play_game
  @player1 = Player.new("X")
  @player2 = Player.new("O")
  # ...
  play_again
end

def play_again
  # ...
  if answer == "yes"
    TicTacToe::Game.new
  else
    # ...
  end
end

You're using recursion to repeat the game, which can get arbitrarily deep, leading to a stack overflow error. Use a loop instead. Also, to avoid violating the Single Responsibility Principle, put this loop outside the Game class whose sole responsibility should be to play a game.

Don't call play_again in play_game, and move play_again outside the class. The main code should be:

def play_again?
  puts "Play again? (yes or no)".yellow
  answer = gets.chomp.downcase
  return answer == "yes"
end

loop do
  TicTacToe::Game.new
  unless play_again?
    puts "Goodbye".cyan.bold
    break
  end
end

(I renamed play_again to play_again?. It's conventional to use a question mark in names of methods that return a boolean)

Unbounded recursion 2

def turn
  @current_turn.even? ? move(@player2) : move(@player1)
end

def move(player)
  while !winning_scenarios && !tie
    # ...
    turn
  end
end

This recursion is not bounded either (it's not bounded by the number of spaces in the board, because a function call occurs for (rejected) invalid moves too). Use a loop instead:

def play_game
  # ...
  while !winning_scenarios && !tie
    turn
  end
  # ...
end

def turn
  @current_turn.even? ? move(@player2) : move(@player1)
end

def move(player)
  # ...
  # Don't call turn here
end

Object oriented design

class Board; ...; end

class Game < Board; ...; end

A game is not a board. A game has a board. Use composition instead of inheritance:

class Game # NOTE: Don't inherit Board
  def initialize
    @board = Board.new # NOTE
    play_game
  end

  def play_game
    # ...
    while [email protected]_scenarios && [email protected]
      turn
    end
    # ...
  end

  def move(player)
    # ...
    space_available = @board.check_space(choice, player.symbol)
    @current_turn += 1 if space_available
    puts "Player #{player.symbol}'s move:".green
    puts @board # NOTE
  end

  def tie_message
    ... if @board.tie
  end

  def win_message
    ... if @board.winning_scenarios
  end

  # ...
end

Note I moved @current_turn += 1 to Game. A game has a current turn, a board doesn't. Modify check_space so it returns a boolean.

Encapsulation

Make internal methods private, so they aren't visible outside the class:

class Board 
  def initialize; ...; end
  def to_s; ...; end
  def check_space(cell, sym); ...; end
  def winning_scenarios; ...; end
  def tie; ...; end

  private

    WINNING_COMBOS = [
      [0, 1, 2], [3, 4, 5], [6, 7, 8],
      [0, 3, 6], [1, 4, 7], [2, 5, 8],
      [0, 4, 8], [2, 4, 6]
    ]

    def place_symbol(cell, sym); ...; end
      spaces[cell] = sym
    end
end

class Game
  def initialize; ...; end
  def play_game; ...; end

  private

    def move(player); ...; end
    def tie_message; ...; end
    def win_message; ...; end
    def turn; ...; end
end

Remove this line which exposes internal fields of Game: (you never actually use Game#symbol, by the way)

attr_reader :player1, :player2, :symbol

And this line from Board: (then change every occurence of spaces in Board to @spaces)

attr_reader :spaces

Board methods

def check_space(cell, sym)
  if @spaces[cell].nil?
    place_symbol(cell, sym)
  else
    puts "Space unavailable! Please select another cell"
  end
end

• Single Responsibility Principle: Board should not do any printing, as it's not its core responsibility. It's better to return a boolean instead and let Game do the printing.
• Naming: This method both checks if the space is free and places a symbol. Rename it to place_symbol_if_free.
• Naming 2: Use position instead of cell to be consistent with other methods.
• Consider splitting this to two methods: space_free?(position) and place_symbol(position, sym) (already exists) and then call both from Game.

WINNING_COMBOS = [
  [0, 1, 2], [3, 4, 5], [6, 7, 8],
  [0, 3, 6], [1, 4, 7], [2, 5, 8],
  [0, 4, 8], [2, 4, 6]
]

Put this inside winning_scenarios, the only method where it's used.

def winning_scenarios
  WINNING_COMBOS.each do |set|
    if @spaces[set[0]] == @spaces[set[1]] && @spaces[set[1]] == @spaces[set[2]]
      return true unless @spaces[set[0]].nil?
    end
  end
  false
end

• Rename winning_scenarios to game_won?. Or to winner and return the winner's symbol
• Use Array#any? instead of looping.
• Use Array#map to obtain the symbols at the locations of a set, instead of repeatedly accessing @spaces.

Result:

def winner
  WINNING_COMBOS.any? do |set|
    symbols = set.map { |position| @spaces[position] }
    if symbols[0] == symbols[1] && symbols[1] == symbols[2]
      symbols[0]
    end
  end
end

def tie
  if [email protected]?(nil) && !winning_scenarios
    return true
  end
end

Rename to tie?, and simply return the boolean:

def tie?
  return [email protected]?(nil) && !winner
end

I also suggets extracting first part to a new private method full?.

to_s can be simplified by using functional programming. See my implementation of as_string in this answer.

Game methods

play_game

puts Board.new

Change this to puts @board.

while [email protected] && [email protected]?

Looks like Board is missing a game_over? method.

win_message
tie_message

These method names are misleading. Each one conditionally prints a message. Merge them to a single print_game_result method.

turn

def turn
  @current_turn.even? ? move(@player2) : move(@player1)
end

The call to move is duplicated because the method does two things: (1) determine who's turn it is (2) call move.

It's better to do only the first one:

def current_player
  @current_turn.even? ? @player2 : @player1
end

Then use move(current_player) in play_game.

move

def move(player)
  puts "Where would you like to move 'player #{player.symbol}'?".red
  choice = gets.chomp.to_i
  space_available = @board.check_space(choice, player.symbol)
  @current_turn += 1 if space_available
  puts "Player #{player.symbol}'s move:".green
  puts @board
end

I'd move the last two puts statements to play_game, because they're not part of making a move.

If you split check_space as I suggest earlier, you can make a loop that gets input until Board#space_free? returns true and then call Board#place_symbol. Otherwise, you'll have to rename this method to try_make_a_move.

initialize

Don't call game_play. Constructors should only initialize an object, and should not do IO. Instead, call game_play in the main code (TicTacToe::Game.new.play_game).

Further Improvements

• Say who won.
• Use digits 1 to 9 when printing the board and getting user input. It's hard to tell 0 and O apart.