3
\$\begingroup\$

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 "Player #{player.symbol}'s move:".green
        puts self
        turn
      end
    end

    def tie_message
      puts "It's a Draw!".cyan if tie
    end

    def win_message
      puts "Game over!".cyan if winning_scenarios
    end

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

    def play_again
      puts "Play again? (yes or no)".yellow
      answer = gets.chomp.downcase
      if answer == "yes"
        TicTacToe::Game.new
      else
        puts "Goodbye".cyan.bold
      end
    end
  end
end

TicTacToe::Game.new
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

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.
\$\endgroup\$
1
  • \$\begingroup\$ Just wow! @Spike thank you so much for pouring through this and making thoughtful comments and suggestions. I will work through your suggestions and make adjustments where necessary. Thanks again. \$\endgroup\$
    – scobo
    Nov 12, 2015 at 18:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.