2 Answers
2

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: