HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Another Tic Tac Toe In Ruby

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
toetictacanotherruby

Problem

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" 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_bor

Solution

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.

Code Snippets

if not Sides.map { |s|
    s.to_s
}.include? first_player_side
if not Sides.include? first_player_side
player_x, player_y = get_players_info

Context

StackExchange Code Review Q#150572, answer score: 4

Revisions (0)

No revisions yet.