patternrubyMinorCanonical
TicTacToe game with AI in ruby - follow-up
Viewed 0 times
withtictactoegamefollowruby
Problem
A week ago I posted my TicTacToe game follow-up question. The suggestions were referring mainly to the lack of polymorphism. Here's the new code, hopefully there's not a lot (or at all) to improve by now (except separating board functionality from Game class, but it seems to be too much of work anyway).
As always, suggestions about structure, logic etc are welcome:
```
# the game board
class Board
attr_accessor :board
def initialize
@board = (1..9).to_a
end
def display_board
puts "\e[H\e[2J" # ANSI clear
@board.each_slice(3).with_index do |row, idx|
print " #{row.join(' | ')}\n"
puts ' ---+---+---' unless idx == 2
end
puts
end
def welcome_msg
print "\nWelcome to Tic Tac Toe.\n\n"
puts 'Enter 1 to play against another player, 2 to play against an evil AI'\
', 3 to watch evil AI play against kind AI.'
puts 'Type EXIT anytime to quit.'
end
def cell_open?(position)
@board[position - 1].is_a?(Fixnum)
end
def win_game?(symbol)
sequences = [[0, 1, 2], [3, 4, 5], [6, 7, 8],
[0, 3, 6], [1, 4, 7], [2, 5, 8],
[0, 4, 8], [2, 4, 6]]
sequences.any? do |seq|
return true if seq.all? { |a| @board[a] == symbol }
end
false
end
def full?
@board.any? do |cell|
return false if cell.is_a? Fixnum
end
true
end
def place_mark(position, symbol)
@board[position - 1] = symbol
end
end
# game logic
class Game
def initialize
@board = Board.new
start_screen
end
def start_screen(choice = nil)
@board.welcome_msg
@player1 = Human.new(@board, 'Player 1', 'X') # defaults
@player2 = AI.new(@board, 'Evil AI', 'O') # defaults
until (1..3).include?(choice)
choice = gets.chomp
exit if choice.downcase == 'exit'
game_modes(choice.to_i)
end
end
def game_modes(choice)
@board.display_board
case choice
when 1 then @player2 = Human.new(@board, 'Player 2', 'O')
when 3
As always, suggestions about structure, logic etc are welcome:
```
# the game board
class Board
attr_accessor :board
def initialize
@board = (1..9).to_a
end
def display_board
puts "\e[H\e[2J" # ANSI clear
@board.each_slice(3).with_index do |row, idx|
print " #{row.join(' | ')}\n"
puts ' ---+---+---' unless idx == 2
end
puts
end
def welcome_msg
print "\nWelcome to Tic Tac Toe.\n\n"
puts 'Enter 1 to play against another player, 2 to play against an evil AI'\
', 3 to watch evil AI play against kind AI.'
puts 'Type EXIT anytime to quit.'
end
def cell_open?(position)
@board[position - 1].is_a?(Fixnum)
end
def win_game?(symbol)
sequences = [[0, 1, 2], [3, 4, 5], [6, 7, 8],
[0, 3, 6], [1, 4, 7], [2, 5, 8],
[0, 4, 8], [2, 4, 6]]
sequences.any? do |seq|
return true if seq.all? { |a| @board[a] == symbol }
end
false
end
def full?
@board.any? do |cell|
return false if cell.is_a? Fixnum
end
true
end
def place_mark(position, symbol)
@board[position - 1] = symbol
end
end
# game logic
class Game
def initialize
@board = Board.new
start_screen
end
def start_screen(choice = nil)
@board.welcome_msg
@player1 = Human.new(@board, 'Player 1', 'X') # defaults
@player2 = AI.new(@board, 'Evil AI', 'O') # defaults
until (1..3).include?(choice)
choice = gets.chomp
exit if choice.downcase == 'exit'
game_modes(choice.to_i)
end
end
def game_modes(choice)
@board.display_board
case choice
when 1 then @player2 = Human.new(@board, 'Player 2', 'O')
when 3
Solution
Board
Whenever I look at code, I first look at the shape and the color. When I look at the code for
There are also a lot literals in there. Perhaps you can extract these and replace them with named constants or methods?
Are you following the SRP? For instance, what does
For
Are you happy with the argument name for
The local variable
I always find that the literals
Game
I found this rather complex to read. A first suggestion would be to use attributes. That will get rid of all those
In
Should all methods be
You are setting up the player defaults in start_screen, only to then potentially change them later. Why not set them once and only once?
The method
I like the name of the method
You might be better of using a plain old
Example code
Here are some mostly complete examples. I feel that there is more room to move stuff around, but they should indicate the things I touched upon. I feel that the
Whenever I look at code, I first look at the shape and the color. When I look at the code for
Board, I find a lot of mixed colors in my color scheme. This suggests to me that maybe you are mixing data with logic.There are also a lot literals in there. Perhaps you can extract these and replace them with named constants or methods?
Are you following the SRP? For instance, what does
welcome_msg have to do with the board? Perhaps this is a little more ambiguous, but what about display_board?For
@board, you are using a 9-element array which seems okay. You might consider making it a 2d-array to make the public interface a little nicer, but I suppose it's fine. But why initialize them with the numbers 1 through 9? It seems to me that the board should be agnostic regarding it's contents. The indices already indicate the positions and having the contents be nil more clearly indicates that it is empty IMHO.Are you happy with the argument name for
#win_game? What about player or player_symbol? Later in the code you use the term mark, so what about mark? What about the method name? Is board supposed to know anything about the rules of the game?The local variable
sequences is really a constant. Consider extracting it. You might also want to break it up into rows, columns and diagonals:ROWS = [[0, 1, 2], [3, 4, 5], [6, 7, 8]]
COLUMNS = [[0, 3, 6], [1, 4, 7], [2, 5, 8]]
DIAGONALS = [[0, 4, 8], [2, 4, 6]]I always find that the literals
true, false and nil are code smells in ruby. This is because ruby expressions are always implicitly truthy or falsey and either nil or not nil. This means that the expression return true if condition can almost always be written more succinctly and more efficiently as just condition (the exception being when you really need true and not just truthy).Game
I found this rather complex to read. A first suggestion would be to use attributes. That will get rid of all those
@ signs :).In
#initialize you are calling start_screen. But start screen has nothing to do with initializing. It is already running the game. Why not move it to the run_game method?Should all methods be
public? What methods do you want clients to call?You are setting up the player defaults in start_screen, only to then potentially change them later. Why not set them once and only once?
The method
game_over is a predicate, so should be named game_over?. The method result? is not a predicate, so should be named result or perhaps something else like display_result.I like the name of the method
check_and_place, but should it be responsible for drawing the board and checking the result as well?You might be better of using a plain old
if-else instead of a case statement in swap_playersExample code
Here are some mostly complete examples. I feel that there is more room to move stuff around, but they should indicate the things I touched upon. I feel that the
Game example class still has to much conditions and to much raw data.class Board
ROWS = [[0, 1, 2], [3, 4, 5], [6, 7, 8]]
COLUMNS = [[0, 3, 6], [1, 4, 7], [2, 5, 8]]
DIAGONALS = [[0, 4, 8], [2, 4, 6]]
def initialize
@cells = Array.new(9)
end
def [](position)
@cells[position - 1]
end
def []=(position, player)
fail RangeError unless (1..@cells.size).include? position
@cells[position - 1] = player
end
def full?
@cells.all?
end
def three_in_a_row?(player)
[ROWS, COLUMNS, DIAGONALS].any? do |sequence|
sequence.all? { |cell| cell == player }
end
end
end
class Game
def initialize
@board = Board.new
end
def run
welcome_message
run_game
end
private
def welcome_message
puts "\nWelcome to Tic Tac Toe.\n"
puts 'Enter 1 to play against another player, 2 to play against an evil AI'\
', 3 to watch evil AI play against kind AI.'
puts 'Type EXIT anytime to quit.'
end
def run_game
p1, p2 = select_players
until game_over?
swap_current_player(p1, p2)
check_and_place
draw_board
end
display_result
end
def select_players
until (1..3).include?(choice = gets.chomp)
exit if choice.downcase == 'exit'
end
case choice
when 1 then [Human.new(@board, 'Player 1', 'X'), Human.new(@board, 'Player 2', 'O')]
when 2 then [Human.new(@board, 'Player 1', 'X'), AI.new(@board, 'Evil AI', 'O')]
when 3 then [AI.new(@board, 'Kind AI, 'X'), AI.new(@board, 'Evil AI, 'O')]
end
end
def game_over?
@board.three_in_a_row(@current_player) || @board.full?
end
def swap_current_player(p1, p2)
@current_player = (@current_player == p1 ? p2 : p1)
end
def display_result
if @board.three_in_a_row?(@current_player.symbol)
puts "Game Over, #{@current_player.name} has won."
else
puts 'Draw.'
end
end
endCode Snippets
ROWS = [[0, 1, 2], [3, 4, 5], [6, 7, 8]]
COLUMNS = [[0, 3, 6], [1, 4, 7], [2, 5, 8]]
DIAGONALS = [[0, 4, 8], [2, 4, 6]]class Board
ROWS = [[0, 1, 2], [3, 4, 5], [6, 7, 8]]
COLUMNS = [[0, 3, 6], [1, 4, 7], [2, 5, 8]]
DIAGONALS = [[0, 4, 8], [2, 4, 6]]
def initialize
@cells = Array.new(9)
end
def [](position)
@cells[position - 1]
end
def []=(position, player)
fail RangeError unless (1..@cells.size).include? position
@cells[position - 1] = player
end
def full?
@cells.all?
end
def three_in_a_row?(player)
[ROWS, COLUMNS, DIAGONALS].any? do |sequence|
sequence.all? { |cell| cell == player }
end
end
end
class Game
def initialize
@board = Board.new
end
def run
welcome_message
run_game
end
private
def welcome_message
puts "\nWelcome to Tic Tac Toe.\n"
puts 'Enter 1 to play against another player, 2 to play against an evil AI'\
', 3 to watch evil AI play against kind AI.'
puts 'Type EXIT anytime to quit.'
end
def run_game
p1, p2 = select_players
until game_over?
swap_current_player(p1, p2)
check_and_place
draw_board
end
display_result
end
def select_players
until (1..3).include?(choice = gets.chomp)
exit if choice.downcase == 'exit'
end
case choice
when 1 then [Human.new(@board, 'Player 1', 'X'), Human.new(@board, 'Player 2', 'O')]
when 2 then [Human.new(@board, 'Player 1', 'X'), AI.new(@board, 'Evil AI', 'O')]
when 3 then [AI.new(@board, 'Kind AI, 'X'), AI.new(@board, 'Evil AI, 'O')]
end
end
def game_over?
@board.three_in_a_row(@current_player) || @board.full?
end
def swap_current_player(p1, p2)
@current_player = (@current_player == p1 ? p2 : p1)
end
def display_result
if @board.three_in_a_row?(@current_player.symbol)
puts "Game Over, #{@current_player.name} has won."
else
puts 'Draw.'
end
end
endContext
StackExchange Code Review Q#110048, answer score: 3
Revisions (0)
No revisions yet.