Ruby Command Line Tic-Tac-Toe

The following is my implementation of a command-line Tic-Tac-Toe game, written in Ruby. This was my first attempt at practicing object-oriented design principles.

```
require 'colored'

module TicTacToe
class Player
attr_accessor :symbol
def initialize(symbol)
@symbol = symbol
end
end

class Board
attr_reader :spaces
def initialize
@spaces = Array.new(9)
end

def to_s
output = ""
0.upto(8) do |position|
output << "#{spaces[position] || position}"
case position % 3
when 0, 1 then output << " | "
when 2 then output << "\n-----------\n" unless position == 8
end
end
output
end

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

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

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

def tie
if !spaces.include?(nil) && !winning_scenarios
return true
end
end
end

class Game < Board
attr_reader :player1, :player2, :symbol
def initialize
super
play_game
end

def play_game
@player1 = Player.new("X")
@player2 = Player.new("O")
puts Board.new
@current_turn = 1
turn
win_message
tie_message
play_again
end

def move(player)
while !winning_scenarios && !tie
puts "Where would you like to move 'player #{player.symbol}'?".red
choice = gets.chomp.to_i
check_space(choice, player.symbol)
puts "P

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 !@board.winning_scenarios && !@board.tie
      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

StackExchange Code Review Q#110480, answer score: 4