Ruby CLI TicTacToe

Question

I have done Tictactoe game in Ruby. It is my first Object Oriented project in Ruby. I would like to separate all program into a few classes such as Player Board and Game. I want to use more OOP best practices.
Also, my move method is looking so big. I would like to refactor my move method because the code is repeating. In a game class, there is 2 break if code piece. Is there any way to reduce it? Lastly, In my module class, do you think that everything is okay or should I use a more concise way?
And also a total review would be great! Thank you for now.

Here is the code. It is working.

module CheckWinner
 def check_horizontal(player1, player2)
 player1_win = false
 player2_win = false
 @board.each do |row|
 player1_win = row.all?(player1)
 player2_win = row.all?(player2)
 break if player1_win || player2_win
 end
 puts "#{player1} won!" if player1_win
 puts "#{player2} won!" if player2_win
 player1_win || player2_win
 end
 def check_vertical(player1, player2)
 player1_win = false
 player2_win = false
 @board.transpose.each do |row|
 player1_win = row.all?(player1)
 player2_win = row.all?(player2)
 break if player1_win || player2_win
 end
 puts "#{player1} won!" if player1_win
 puts "#{player2} won!" if player2_win
 player1_win || player2_win
 end
 def check_diagonal(player1, player2)
 if @board[0][0] == player1 && board[1][1] == player1 && board[2][2] == player1 ||
 @board[0][2] == player1 && board[1][1] == player1 && board[2][0] == player1
 puts "#{@player1} won!"
 true
 elsif @board[0][0] == player2 && board[1][1] == player2 && board[2][2] == player2 ||
 @board[0][2] == player2 && board[1][1] == player2 && board[2][0] == player2
 puts "#{@player2} won!"
 true
 end
 end
end
# TicTacToe Board
class Board
 include CheckWinner
 attr_accessor :board
 def initialize
 @board = [[1, 2, 3], [4, 5, 6], [7, 8, 9]]
 end
 def print_board
 puts '-------------'
 @board.each do |row|
 print '|'
 row.each do |col|
 print " #{col}"
 print ' | '
 end
 puts
 end
 puts '-------------'
 end
 def occupied_error(value)
 puts 'There is a value or wrong place! Try Again!'
 print_board
 value == @player1 ? player2(@player1) : player1(@player2) # Stay same player
 end
 def move(place, value) # Place : 1-9, value = X || Y
 case place
 when 1
 if @board[0][0] == 'X' || @board[0][0] == 'O'
 occupied_error(value)
 else
 @board[0][0] = value
 end
 when 2
 if @board[0][1] == 'X' || @board[0][1] == 'O'
 occupied_error(value)
 else
 @board[0][1] = value
 end
 when 3
 if @board[0][2] == 'X' || @board[0][2] == 'O'
 occupied_error(value)
 else
 @board[0][2] = value
 end
 when 4
 if @board[1][0] == 'X' || @board[1][0] == 'O'
 occupied_error(value)
 else
 @board[1][0] = value
 end
 when 5
 if @board[1][1] == 'X' || @board[1][1] == 'O'
 occupied_error(value)
 else
 @board[1][1] = value
 end
 when 6
 if @board[1][2] == 'X' || @board[1][2] == 'O'
 occupied_error(value)
 else
 @board[1][2] = value
 end
 when 7
 if @board[2][0] == 'X' || @board[2][0] == 'O'
 occupied_error(value)
 else
 @board[2][0] = value
 end
 when 8
 if @board[2][1] == 'X' || @board[2][1] == 'O'
 occupied_error(value)
 else
 @board[2][1] = value
 end
 when 9
 if @board[2][2] == 'X' || @board[2][2] == 'O'
 occupied_error(value)
 else
 @board[2][2] = value
 end
 else
 puts 'It is not a valid value,please try again!(0..9)'
 occupied_error(value)
 end
 end
 def full?
 if @board.flatten.all?(String)
 puts "Draw!"
 true
 end
 end
 def choosing_player
 puts 'Choose for Player1(X or O)'
 loop do
 @player1 = gets.chomp!
 break if @player1 == 'X' || @player1 == 'O'
 puts 'Try Again!(X or O)'
 end
 puts "Player 1 is: #{@player1}"
 @player1 == "X" ? @player2 = "O" : @player2 = "X"
 puts "Player 2 is: #{@player2}"
 print_board
 end
 def player1(player1)
 puts "Choice #{player1} Place on a board(1 to 10)"
 @place = gets.chomp!.to_i
 move(@place, player1)
 print_board
 end
 def player2(player2)
 puts "Choice #{player2} Place on a board(1 to 10)"
 @place = gets.chomp!.to_i
 move(@place, player2)
 print_board
 end
 def game
 choosing_player
 loop do
 player1(@player1)
 break if check_vertical(@player1, @player2) == true || check_diagonal(@player1, @player2) == true || check_horizontal(@player1, @player2) == true || full?
 player2(@player2)
 break if check_vertical(@player1, @player2) == true || check_diagonal(@player1, @player2) == true || check_horizontal(@player1, @player2) == true || full?
 end
 end
end
board = Board.new
board.game

Repl it repo : https://replit.com/@Burakkepuc/TicTacToe#main.rb

• Toby Speight: Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again.

Answer 1

Welcome to Code Review, you have almost the same code block repeated 9 times:

 if @board[0][2] == 'X' || @board[0][2] == 'O'
 occupied_error(value)
 else
 @board[0][2] = value
 end

So write a function and use it instead:

def assign_if_possible(x, y, value)
 if @board[y][x] == 'X' || @board[y][x] == 'O'
 occupied_error(value)
 else
 @board[y][x] = value
 end

and use it instead. Also using the % modulo operator should allow you to avoid the case statement altogether, something like (x = val % 3, y = (val / 3).to_i), the exact expression should not be too hard to find.

• theplaceofburak: Thank you I changed the code, but I hardcoded it in an ` assign_if_possible` parameters. Am I correct ? So I mean inside the move method ` when 1 assign_if_possible(0,0,value) `
• Caridorc: @theplaceofburak yes, that is the simpler but longer possibility, otherwise you can use the % operator as said here to go from 1d coordinate to 2d coordinates to avoid the switch statement stackoverflow.com/questions/5494974/…
• theplaceofburak: Thank you for your answer.

Answer 2

Simplifying Board referencing will profoundly simplify the using code.

@board = [[1, 2, 3], [4, 5, 6], [7, 8, 9]]
#using actual coordinates
@diagonals = [
 [@board[0][0], @board[1][1], @board[2][2]],
 [@board[0][2], @board[1][1], @board[2][0]], 
 ]
@columns = [
 [@board[0][0], @board[1][0], @board[2][0]],
 [@board[0][1], @board[1][1], @board[2][1]], 
 [@board[0][2], @board[1][2], @board[2][2]], 
 ]
@rows = [
 [@board[0][0], @board[0][1], @board[0][2]],
 [@board[1][0], @board[1][1], @board[1][2]], 
 [@board[2][0], @board[2][1], @board[2][2]], 
 ]
def winner? (player) { # 'X' or 'O'
 won = false
 won = @[email protected] { |row| row.all?(player) ? true : won }
 won = @[email protected] { |diag| diag.all?(player) ? true : won }
 won = @[email protected] { |col| col.all?(player) ? true : won }
 won
end #winner? 

---

The program should accept lower case "x" or "o" and change it without making the user re-enter. And same consideration if user enters zero instead of oh. Or is it vice versa?

---

Use integers as Hash keys

Then mapping [[1, 2, 3], [4, 5, 6], [7, 8, 9]] to the board will subdue that awful case statement

@user_entry = {
 1 => @board[0][0],
 2 => @board[0][1],
 3 => @board[0][2]
 # et cetera
 }

Bonus! Now the board can be initialized with nil and still have user enter 1 - 9. A consistent initial value will simplify more code.

def move(player, entry) {
 @user_entry[entry] = player unless occupied?(@user_entry[entry])
end
def occupied?(this_square)
 return @user_entry[this_square].nil? ? false : true
end