patternrubyMinor
TicTacToe with AI in ruby - follow-up overload
Viewed 0 times
withtictactoeoverloadfollowruby
Problem
I already posted this program numerous times, always surprised by the amount of improvement that is possible suggested by people here. Is it possible to make it even better, without nitpicking? At this point this is a special curiosity of mine =).
```
# win sequences constant
WIN_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]]
# the game board
class Board
attr_accessor :cells
def initialize
@cells = Array.new(9) # { nil }
end
def as_string
stringed_board = @cells.map.with_index do |symbol, idx|
symbol || idx + 1
end
stringed_board.each_slice(3).map do |row|
" #{row.join(' | ')}"
end.join("\n ---+---+---\n")
end
def cell_open?(position)
@cells[position - 1].nil?
end
def win_game?(symbol)
WIN_SEQUENCES.any? do |seq|
return true if seq.all? { |a| @cells[a] == symbol }
end
end
def full?
@cells.all?
end
def place_symbol(position, symbol)
@cells[position - 1] = symbol
end
end
# game logic
class Game
def initialize
@board = Board.new
# default states
@player1 = Human.new(@board, 'Player 1', 'X')
@player2 = AI.new(@board, 'Evil AI', 'O')
welcome_msg
start_screen
end
private
def start_screen(choice = gets.chomp)
until (1..3).include?(choice)
exit if choice.downcase == 'exit'
select_game_mode(choice.to_i)
end
end
def select_game_mode(choice)
case choice
when 1 then [@player2 = Human.new(@board, 'Player 2', 'O')]
when 3 then [@player1 = AI.new(@board, 'Kind AI', 'X'),
@player2 = AI.new(@board, 'Evil AI', 'O')]
else puts 'You silly goose, try again.'
end
@current_player = @player2
display_board
run_game
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 k
```
# win sequences constant
WIN_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]]
# the game board
class Board
attr_accessor :cells
def initialize
@cells = Array.new(9) # { nil }
end
def as_string
stringed_board = @cells.map.with_index do |symbol, idx|
symbol || idx + 1
end
stringed_board.each_slice(3).map do |row|
" #{row.join(' | ')}"
end.join("\n ---+---+---\n")
end
def cell_open?(position)
@cells[position - 1].nil?
end
def win_game?(symbol)
WIN_SEQUENCES.any? do |seq|
return true if seq.all? { |a| @cells[a] == symbol }
end
end
def full?
@cells.all?
end
def place_symbol(position, symbol)
@cells[position - 1] = symbol
end
end
# game logic
class Game
def initialize
@board = Board.new
# default states
@player1 = Human.new(@board, 'Player 1', 'X')
@player2 = AI.new(@board, 'Evil AI', 'O')
welcome_msg
start_screen
end
private
def start_screen(choice = gets.chomp)
until (1..3).include?(choice)
exit if choice.downcase == 'exit'
select_game_mode(choice.to_i)
end
end
def select_game_mode(choice)
case choice
when 1 then [@player2 = Human.new(@board, 'Player 2', 'O')]
when 3 then [@player1 = AI.new(@board, 'Kind AI', 'X'),
@player2 = AI.new(@board, 'Evil AI', 'O')]
else puts 'You silly goose, try again.'
end
@current_player = @player2
display_board
run_game
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 k
Solution
I'll try to analyze each class in your code separately.
Duplication in classes
Class
Overall, try not to mix business logic in your code with input/output functionality.
That's it for now. Will update my answer later.
Board - update certain methods:def as_string
stringed_board = @cells.map.with_index(1) { |symbol, index| symbol || index }
#.....
end
def cell_open?(position)
!@cells[position - 1] # personal preference to use '!'
end
def win_game?(symbol)
WIN_SEQUENCES.any? do |seq|
seq.all? { |c| @cells[c] == symbol }
# not necessary to return true explicitly
end
endDuplication in classes
Human and AI(attr_readers and initialize method). I suggest to move it to parent class (call it Player):class Player
attr_reader :name, :symbol
def initialize(board, name, symbol)
@board, @name, @symbol = board, name, symbol
end
end
class Human < Player
# code without initialize method and attr_accessors
end
class AI < Player
attr_reader :board
# code ...
endAI class:class AI < Player
attr_reader :board
def take_input
loading_simulation
check_win_or_block(symbol) || check_win_or_block(other_symbol) || check_defaults
# there was a lot of duplication here (with unnecessary explicit returns)
end
private
def check_win_or_block(sym)
# check_win and check_block have same logic and
# can be replaced with one method
finished = false
0.upto(8) do |i|
origin = board.cells[i]
board.cells[i] = sym if !origin
finished = i + 1 if board.win_game?(sym)
board.cells[i] = origin
end
finished
end
def other_symbol
symbol == 'X' ? 'O' : 'X' # I'm using ternary operator in such cases
end
def check_defaults
# @finished variable is unnecessary now
# possible_slides and possible_corners are pieces with the same logic
# I replace them with possible_position that passes block
if board.cells[4]
rand < 0.51 ? possible_position(&:even?) : possible_position(&:odd?)
else
5
end
end
def possible_position(&block)
result = (1..9).select(&block).each do |i|
return i + 1 if !board.cells[i]
end
result.is_a?(Integer) ? result : board.cells.rindex(nil) + 1
# this line takes functionality of your code (take_input method):
# (1..9).reverse_each { |i| return i if board.cells[i - 1].nil? }
end
def loading_simulation
str = "\r#{name} is scheming"
10.times { print str += '.'; sleep(0.1) }
end
endClass
Game:class Game
def initialize
# .....
@player2 = AI.new(@board, 'Evil AI', 'O')
@current_player = @player2
# current_player is not a part of select_game_mode method functionality
welcome_msg
start_screen
end
private
def start_screen(choice = gets)
choice.strip!.downcase! if choice
# In your code use until loop is not working. Reason:
# 'gets' must be placed inside of this loop
until %w(1 2 3 exit).include?(choice)
# (1..3).include?(choice) in your code will always return false
# because choice is a String
puts 'You silly goose, try again.'
start_screen(gets)
end
select_game_mode(choice)
display_board
run_game
end
def select_game_mode(choice)
case choice
when '1' then @player2 = Human.new(@board, 'Player 2', 'O')
when '3' then @player1 = AI.new(@board, 'Kind AI', 'X')
# @player2 = ... removed - it's already set.
when 'exit' then exit
# exit option must be here (I think).
end
end
# ....
def run_game
until game_over?
swap_players
check_and_place
display_board
if game_over?
puts display_result
exit
# exit moved from display_result. It changes state of program but not displaying result
end
end
end
# .....
def display_result
# It's "bad" style to place business logic and UI in one method.
# code is not clear and harder to test
# all puts's moved to run_game_method
if @board.win_game?(@current_player.symbol)
"Game Over, #{@current_player.name} has won."
elsif @board.full?
'Draw.'
end
end
def swap_players
@current_player = @current_player == @player1 ? @player2 : @player1
# It's not essentially, but looks better.
end
endOverall, try not to mix business logic in your code with input/output functionality.
That's it for now. Will update my answer later.
Code Snippets
def as_string
stringed_board = @cells.map.with_index(1) { |symbol, index| symbol || index }
#.....
end
def cell_open?(position)
!@cells[position - 1] # personal preference to use '!'
end
def win_game?(symbol)
WIN_SEQUENCES.any? do |seq|
seq.all? { |c| @cells[c] == symbol }
# not necessary to return true explicitly
end
endclass Player
attr_reader :name, :symbol
def initialize(board, name, symbol)
@board, @name, @symbol = board, name, symbol
end
end
class Human < Player
# code without initialize method and attr_accessors
end
class AI < Player
attr_reader :board
# code ...
endclass AI < Player
attr_reader :board
def take_input
loading_simulation
check_win_or_block(symbol) || check_win_or_block(other_symbol) || check_defaults
# there was a lot of duplication here (with unnecessary explicit returns)
end
private
def check_win_or_block(sym)
# check_win and check_block have same logic and
# can be replaced with one method
finished = false
0.upto(8) do |i|
origin = board.cells[i]
board.cells[i] = sym if !origin
finished = i + 1 if board.win_game?(sym)
board.cells[i] = origin
end
finished
end
def other_symbol
symbol == 'X' ? 'O' : 'X' # I'm using ternary operator in such cases
end
def check_defaults
# @finished variable is unnecessary now
# possible_slides and possible_corners are pieces with the same logic
# I replace them with possible_position that passes block
if board.cells[4]
rand < 0.51 ? possible_position(&:even?) : possible_position(&:odd?)
else
5
end
end
def possible_position(&block)
result = (1..9).select(&block).each do |i|
return i + 1 if !board.cells[i]
end
result.is_a?(Integer) ? result : board.cells.rindex(nil) + 1
# this line takes functionality of your code (take_input method):
# (1..9).reverse_each { |i| return i if board.cells[i - 1].nil? }
end
def loading_simulation
str = "\r#{name} is scheming"
10.times { print str += '.'; sleep(0.1) }
end
endclass Game
def initialize
# .....
@player2 = AI.new(@board, 'Evil AI', 'O')
@current_player = @player2
# current_player is not a part of select_game_mode method functionality
welcome_msg
start_screen
end
private
def start_screen(choice = gets)
choice.strip!.downcase! if choice
# In your code use until loop is not working. Reason:
# 'gets' must be placed inside of this loop
until %w(1 2 3 exit).include?(choice)
# (1..3).include?(choice) in your code will always return false
# because choice is a String
puts 'You silly goose, try again.'
start_screen(gets)
end
select_game_mode(choice)
display_board
run_game
end
def select_game_mode(choice)
case choice
when '1' then @player2 = Human.new(@board, 'Player 2', 'O')
when '3' then @player1 = AI.new(@board, 'Kind AI', 'X')
# @player2 = ... removed - it's already set.
when 'exit' then exit
# exit option must be here (I think).
end
end
# ....
def run_game
until game_over?
swap_players
check_and_place
display_board
if game_over?
puts display_result
exit
# exit moved from display_result. It changes state of program but not displaying result
end
end
end
# .....
def display_result
# It's "bad" style to place business logic and UI in one method.
# code is not clear and harder to test
# all puts's moved to run_game_method
if @board.win_game?(@current_player.symbol)
"Game Over, #{@current_player.name} has won."
elsif @board.full?
'Draw.'
end
end
def swap_players
@current_player = @current_player == @player1 ? @player2 : @player1
# It's not essentially, but looks better.
end
endContext
StackExchange Code Review Q#114888, answer score: 4
Revisions (0)
No revisions yet.