# Another Tic Tac Toe In Ruby

I am messing with Ruby a little bit and would like to get some feedback about the Spaghettii below :)

 Sides = ["X", "O"]

Msg_for_restart = " Starting all over again"
Msg_for_coordinates = "Enter coordinates in the form of x,y"
Msg_for_matrix = "This is how the matrix looks like"
Draw_msg = "The game is draw, try again please."

Rows, Columns = 3, 3

class Player
attr_reader :name, :side
def initialize(name, side)
@name, @side = name, side
end

def to_s
"#{@name} with #{@side} side"
end
end

def initialize_matrix(rows, columns)
Array.new(rows) { Array.new(columns) }
end

def print_matrix(matrix)
puts "----"
matrix.each_with_index do |row_value, i|
row_value.each_with_index do |column_value, j|
print matrix[i][j].to_s + " "
end
puts
end
puts "----"
end

def get_input
$stdin.gets.chomp end def get_players_info puts "Welcome to Tic Tac Toe Game" puts "Enter the name for the first player: " first_player_name = get_input puts "Enter a side to play with " first_player_side = get_input if not Sides.map { |s| s.to_s }.include? first_player_side puts "Sorry, only X and O values are allowed" << Msg_for_restart get_players_info end puts "Enter the name for the second player: " second_player_name = get_input puts "Enter a side for the second player: " second_player_side = get_input if first_player_side.to_s == second_player_side puts "Sorry, sides can't be the same" << Msg_for_restart get_players_info end [Player.new(first_player_name, first_player_side), Player.new(second_player_name, second_player_side)] end def get_x_player(players) return players[0] if players[0].side == Sides[0].to_s players[1] end def get_y_player(players) return players[0] if players[0].side == Sides[1].to_s players[1] end def are_elements_equal(array, element) begin array.each do |el| return false if el != element end return true rescue return false end false end def is_player_winning_based_on_rows(side, matrix) matrix.each_with_index do |x, xi| row = [] x.each_with_index do |y, yi| row << matrix[xi][yi] end return true if are_elements_equal(row, side) end false end def is_player_winning_based_on_columns(side, matrix) matrix.each_with_index do |x, xi| row = [] x.each_with_index do |y, yi| row << matrix[yi][xi] end return true if are_elements_equal(row, side) end false end def is_player_winning_based_on_diagonals(side, matrix) return true if matrix[0][0] == side && matrix[0][0] && matrix[1][1] && matrix[1][1] == matrix[2][2] || \ matrix[2][0] == side && matrix[2][0] == matrix[1][1] && matrix[1][1] == matrix[0][2] false end matrix = initialize_matrix(Rows, Columns) row_border = Rows - 1 column_border = Columns - 1 players = get_players_info x_player = get_x_player(players) y_player = get_y_player(players) while true print_matrix(matrix) puts "Hello #{x_player.to_s}" puts Msg_for_coordinates x_player_input = get_input.split(",") raise IndexError if x_player_input[0].to_i > row_border || x_player_input[1].to_i > column_border matrix[x_player_input[0].to_i][x_player_input[1].to_i]= Sides[0] if is_player_winning_based_on_rows(x_player.side, matrix) || is_player_winning_based_on_columns(x_player.side, matrix) || is_player_winning_based_on_diagonals(x_player.side, matrix) puts "Congatulations #{x_player.to_s}, You Won!" break end print_matrix(matrix) puts "Hello #{y_player.to_s}" puts Msg_for_coordinates y_player_input = get_input.split(",") raise IndexError if y_player_input[0].to_i > row_border || y_player_input[1].to_i > column_border matrix[y_player_input[0].to_i][y_player_input[1].to_i]= Sides[1] print_matrix(matrix) if is_player_winning_based_on_rows(y_player.side, matrix) || is_player_winning_based_on_columns(y_player.side, matrix) || is_player_winning_based_on_diagonals(y_player.side, matrix) puts "Congatulations #{y_player.to_s}, You Won!" break end end  ## 2 Answers ### Input validation Input validation with recursion is kind of ugly. A user could crash the program by repeatedly entering invalid values until the call stack overflows. I recommend to rewrite the input validation in get_players_info to use a loop instead of recursion. ### Usability I see a couple of usability issues in get_players_info: • From the message "Enter a side to play with " it's not obvious what are the valid values. It would be nice to include them in the message. • If the second user chooses invalid side, we have to start the input all over again, from the name of player 1, and so on. It would be friendlier to repeat from the last invalid point. • Why offer the choice of side for the second player at all, when there is only one valid choice? ### Performance is_player_winning_based_on_rows builds an array of values for each row, and then checks if all values are equal to side. That's kind of a waste if the first value is not equal to side. It would be better to stop iterating immediately in that case. The same issue in is_player_winning_based_on_columns too. ### Opportunities to simplify Instead of this: if not Sides.map { |s| s.to_s }.include? first_player_side  How about this: if not Sides.include? first_player_side  Instead of the get_x_player and get_y_player functions, you could change get_players_info to return player_x, player_y, and call it with: player_x, player_y = get_players_info  I don't know much ruby, but I don't understand why the rescue statement is necessary in the are_elements_equal function. # Marked up code Rather than extracting small lines of code and commenting on them, instead I've added many inline comments about your code. I've also modified it in several places, trying to keep your logic intact where possible, and I've added comments about the reasons for my changes. # constants should be all upper case MSG_ENTER_COORDINATES = "Enter coordinates in the form of x,y" MSG_BOARD_VIEW = "This is how the matrix looks like" MSG_DRAW_GAME = "The game is draw, try again please." NUM_ROWS, NUM_COLUMNS = 3, 3 # SIDES = ["X", "O"] # As an array this makes things look ugly. Using X and O constants or tokens would be better # Also with the changes I've added, these arent even necessary anymore class Player attr_reader :name, :side def initialize(name, side) @name, @side = name, side end def to_s "#{@name} with #{@side} side" end end # I've moved all of your 'matrix' functions into a class and called it Board class Board attr_reader :board # was initialize_board def initialize(rows, columns) @board = Array.new(rows) { Array.new(columns, ' ') } # Initialize all indexes to a space. # This will fix your print issue where the rows are different lengths when printing end # was print_board def to_s str = [] str << "-----" # I added an extra '-' so that it is the same width as the board str << @board.map { |row| row.join(' ') } # Does the same thing without an inner loop str << "-----" str.join("\n") end # NEW! def is_player_winning(side) is_player_winning_based_on_rows(side) || is_player_winning_based_on_columns(side) || is_player_winning_based_on_diagonals(side) end # NEW! def record_move(coordinates, side) #corrdinates are expected to be in <x, y> (col, row) format (as strings) @board[coordinates.first.to_i][coordinates.last.to_i] = side end private def are_elements_equal(array, element) # your previous implementation was way more complex than it needed to be array.all? { |e| e == element } end def is_player_winning_based_on_rows(side) # again your logic can be simplified @board.any? { |row| are_elements_equal(row, side) } end def is_player_winning_based_on_columns(side) #same as above, but transposed @board.transpose.any? { |row| are_elements_equal(row, side) } end def is_player_winning_based_on_diagonals(side) # I think this is a little cleaner [@board[0][0], @board[1][1], @board[2][2]].all? { |e| e == side } || [@board[0][2], @board[1][1], @board[2][0]].all? { |e| e == side } end end def get_input$stdin.gets.chomp # fyi, when running from the command line, 'gets' will automatically read from \$stdin
end

def get_players_info
puts "Welcome to Tic Tac Toe Game"
puts "Enter the name for the first player: "
first_player_name = get_input
puts "Enter the name for the second player: "
second_player_name = get_input

# You always have player X go first, so rather than prompting for a side for each player, just ask who goes first and make that player X
first_player_index = nil
until first_player_index == '1' || first_player_index == '2'
puts "Who plays first? (Enter '1' or '2')"
puts "1) #{first_player_name}"
puts "2) #{second_player_name}"
first_player_index = get_input
end

if first_player_index == '1'
[Player.new(first_player_name, "X"),  Player.new(second_player_name, "O")]
else
[Player.new(second_player_name, "X"),  Player.new(first_player_name, "O")]
end
end

# UNUSED
# def get_x_player(players)
# def get_y_player(players)

# NEW!
def get_player_move(board, player)
# this function returns true if game over,, false otherwise

puts board                    # was print_board(matrix)
puts "Hello #{player}"        # to_s will be called automatically

# this is very poor error handling
# raise IndexError if player_input[0].to_i >= NUM_ROWS || player_input[1].to_i >= NUM_COLUMNS # used constants and chaged logic to greater-than-or-equals

# Better way to get player input: use a regex to ensure that only valid digits have been entered
# This regex looks for the digit 0,1,2 followed by a comman, followed by 0,1,2.  Optional whitespace is supported
player_input = nil
until player_input do
puts MSG_ENTER_COORDINATES
player_input = get_input.strip.match /([012])\s*,\s*([012])/ # player_input is a Match object.  Evaluates to false if no matches found
end

board.record_move(player_input.captures, player.side)  # was matrix[x_player_input[0].to_i][x_player_input[1].to_i]= SIDES[0]

if board.is_player_winning(player.side)
puts "Congatulations #{player}, You Won!"  # to_s is called automatically
return true
end

false
end

board = Board.new(NUM_ROWS, NUM_COLUMNS)

# Ruby will automatically dereference an array.  It is not necessary to break this into mutiple statements
x_player, y_player = get_players_info

while true
# this is just the same logic for both players
# instead, I have moved the logic for a single player into the 'get_player_move' function and we call it twice
break if get_player_move(board, x_player)
break if get_player_move(board, y_player)
end


# Additional Thoughts

As janos mentioned, your program has several usability issues. While I've cleaned it up some a little and added better input validation, there is still a lot to be desired. Making the user enter coordinates, especially without adding row and column headers is hard on the user. It would be better to either add headers, or even better, add a guide with numbers and letters off to the side. For example:

                Please select a cell:
X |   | O    ||      | 2 |
---+---+---   ||   ---+---+---
| X |      ||    4 |   | 6
---+---+---   ||   ---+---+---
|   |      ||    7 | 8 | 9


This would allow a player to only enter a single digit AND have a visual guide.

You could also make other visual changes, like drawing the board grid lines like I have (or even using extended ascii).

Finally, you have one major design issue, in that you allow a players to overwrite each other. There is no logic to prevent a player from playing in a cell that has already been used.