Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/

edited Apr 13, 2017 at 12:40

Community Bot

• 1

Inline Side-by-side Side-by-side Markdown

answered Nov 12, 2015 at 16:08

Spike

• 1.5k
• 8
• 15

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.