Another Tic Tac Toe In Ruby

Question

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

Answer 1

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.

Answer 2

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.