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.
