8
\$\begingroup\$

After reading a lot of tutorials, I was finally able to code my version of a Tic-Tac-Toe game. I would like to have some honest review of the code to see what I can do to better my coding.

 class  Game

 attr_reader :board , :marker

def initialize 
    @board  = Array.new(3," "){Array.new(3," ")}
    @marker = ["X", "O"]
    #array that will be use to track all player move 
    #and use to find if there's any empty space on the board 
    @track_move ||= []
end

def create_players
    # create both players 
    @names = []

    @player_1 = ask "Please enter the first player name: "
    @names.push(@player_1)

    @player_2 = ask "Please enter second player name: "
    @names.push(@player_2)

    space
    puts"welcome #{@player_1.upcase} and #{@player_2.upcase}"
    print_separator
    space

    puts "Randomizing who'll start..."
    sleep(1)
    space
    # assign player by calling the player_assigment function that will determine who will start first
    player_assigment
    space
end

def space
    puts"\n"
end

def ask(question, kind="string")
    print question + " "
    answer = gets.chomp.downcase
    answer = answer.to_i if kind == "number"
    return answer 
end

def print_separator(character="-")
    puts character * 49
end

def player_assigment
    # merge the names array and symbol array 
    # with the zip method and return a nested array with player and symbol.  
    @choice = @names.shuffle.zip(marker)
    # iterate over the choice nested array and 
    # print out each player and their assigned symbol
    @assign = @choice.to_h
    @assign.each do |player, symbol| 
     puts "#{player.upcase} symbol is #{symbol}" 
    end
end

def first
    # choose the first array in choice nested array and return the first player and symbol
    first = @choice.first
    @first_player = first.first
    @symbol = first.last
    @current = @first_player
end

def second
    # choose the second array in choice nested array and return the second player and symbol
    second = @choice.last
    @second_player = second.first
    @symbol = second.last
    @current = @second_player
end

def first_turn
    first
    puts "it's #{@first_player.upcase} with '#{@symbol}' turn"
    make_move
end

def next_turn
    second
    puts "#{@second_player.upcase} with '#{@symbol}' turn"
    make_move
end

def invalid
    puts"Invalid move"
    space
end

def make_move
    @move = ask("Where to:","number")
    space
    mapping(@move,@symbol)
end

def track_move(move)
    # each move will be assigned an integer value that 
    # will be used to determine if there's no more move left.
    if @current 
       @track_move.push(move)
    end
end

def process_move(move)
    print_board
    track_move(move)
    if move_left?
       end_print
       replay
    end
    check_win?
end

def mapping(move, symbol)
     case move
        when 0
            if board[0][0]=="X" || @board[0][0] == "O"
               invalid
               make_move
            else
               board[0][0] = symbol
               process_move(@move)
            end

        when 1
            if board[0][1]=="X" || board[0][1] == "O"
               invalid
               make_move
            else
              board[0][1] = symbol
              process_move(@move)
            end

        when 2
            if board[0][2]=="X" || board[0][2] == "O"
               invalid
               make_move
            else
               board[0][2] = symbol
               process_move(@move)
            end

        when 3
            if board[1][0]=="X" || board[1][0] == "O"
               invalid
               make_move
            else
               board[1][0] = symbol
               process_move(@move)
            end

        when 4
            if board[1][1]=="X" || board[1][1] == "O"
               invalid
               make_move
            else
               board[1][1] = symbol
               process_move(@move)
            end

        when 5
            if board[1][2]=="X" || board[1][2] == "O"
               invalid
               make_move
            else
               board[1][2] = symbol
               process_move(@move)
            end

        when 6
            if board[2][0]=="X" || board[2][0] == "O"
               invalid
               make_move
            else
               board[2][0] = symbol
               process_move(@move)
            end

        when 7
            if board[2][1]=="X" || board[2][1] == "O"
               invalid
               make_move
            else
               board[2][1] = symbol
               process_move(@move)
            end

        when 8
            if board[2][2]=="X" || board[2][2] == "O"
               invalid
               make_move
            else
               board[2][2] = symbol
               process_move(@move)
            end

        else 
               invalid
               make_move
    end
end

def end_print
    print_separator
    puts "Game over. ".upcase 
    puts "No more moves left. "
    print_separator
    space
end


def winner(player)
    print_separator
    puts"#{player.upcase} wins!! TICTACTOE"
    puts "Game over. ".upcase
    print_separator
    space
    replay
end

def replay      
    print "Do you want to play again?(y/n)"
    space
    ans = gets.chomp
    if ans == "y"
    Game.start
    else
        exit
    end
end

def move_left?
    sum  = @track_move.inject{|r, s| r + s}
    if sum == 36
       return true
    end
end

def check_win?
    player = @current
    symbol = @symbol
    if board[0][0] == symbol && 
       board[0][1] == symbol &&
       board[0][2] == symbol
       winner(player)

    elsif
       board[1][0] == symbol && 
       board[1][1] == symbol &&
       board[1][2] == symbol
       winner(player)

    elsif
       board[2][0] == symbol && 
       board[2][1] == symbol &&
       board[2][2] == symbol
       winner(player)

    elsif
       board[0][1] == symbol && 
       board[1][1] == symbol &&
       board[2][1] == symbol
       winner(player)

    elsif
       board[0][2] == symbol && 
       board[1][2] == symbol &&
       board[2][2] == symbol
       winner(player) 

    elsif
       board[0][2] == symbol && 
       board[1][1] == symbol &&
       board[2][0] == symbol
       winner(player) 

    elsif
       board[0][0] == symbol && 
       board[1][1] == symbol &&
       board[2][2] == symbol
       winner(player)

    end
     return false
end

# display board in a matrix format 
def print_board
    board.each_slice(1) { |a| p a }
    space
end


def instructions
    print_separator
    puts "Instructions :Enter your first move by entering a number 1-9" 
    puts "corresponding to the grid on the bottom and press enter"
    print_separator
    space
    puts "0 | 1 | 2 ",             
         "----------",              
         "3 | 4 | 5 ",             
         "----------",            
         "6 | 7 | 8 " 
    space

end

def players_turn
    loop do
        first_turn
        next_turn
    end
end

def self.start
    puts"------------------------------------------------"
    puts"              Welcome to tictactoe              ".upcase
    puts"------------------------------------------------"
    print"\n"
    new_game = Game.new
    new_game.create_players
    new_game.instructions
    new_game.players_turn
end

Game.start

end
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to codereview! Could you please indent your code exactly in the way it looks in your ide? \$\endgroup\$ – Caridorc Aug 29 '15 at 7:29
6
\$\begingroup\$

When seeking improvement of a piece of code, the first thing is removing repetition:

def mapping(move, symbol)
     case move
        when 0
            if board[0][0]=="X" || @board[0][0] == "O"
               invalid
               make_move
            else
               board[0][0] = symbol
               process_move(@move)
            end

        when 1
            if board[0][1]=="X" || board[0][1] == "O"
               invalid
               make_move
            else
              board[0][1] = symbol
              process_move(@move)
            end

        when 2
            if board[0][2]=="X" || board[0][2] == "O"
               invalid
               make_move
            else
               board[0][2] = symbol
               process_move(@move)
            end

        when 3
            if board[1][0]=="X" || board[1][0] == "O"
               invalid
               make_move
            else
               board[1][0] = symbol
               process_move(@move)
            end

        when 4
            if board[1][1]=="X" || board[1][1] == "O"
               invalid
               make_move
            else
               board[1][1] = symbol
               process_move(@move)
            end

        when 5
            if board[1][2]=="X" || board[1][2] == "O"
               invalid
               make_move
            else
               board[1][2] = symbol
               process_move(@move)
            end

        when 6
            if board[2][0]=="X" || board[2][0] == "O"
               invalid
               make_move
            else
               board[2][0] = symbol
               process_move(@move)
            end

        when 7
            if board[2][1]=="X" || board[2][1] == "O"
               invalid
               make_move
            else
               board[2][1] = symbol
               process_move(@move)
            end

        when 8
            if board[2][2]=="X" || board[2][2] == "O"
               invalid
               make_move
            else
               board[2][2] = symbol
               process_move(@move)
            end

        else 
               invalid
               make_move
    end
end

Just the numbers are changing, the actions are the same. We can take advantage of this to shorten this piece of code.

def mapping(move, symbol)
     input_to_coords = { 0 : [0, 0], ... 8 : [2, 2] } # Fill in the ellipsis
     x, y = input_to_coords[move]
     if board[x][y]=="X" || board[x][y] == "O"
           invalid
           make_move
        else
           board[x][y] = symbol
           process_move(@move)
        end
  end

You can also reduce the repetition in check_win

def check_win?
player = @current
    symbol = @symbol
    if board[0][0] == symbol && 
       board[0][1] == symbol &&
       board[0][2] == symbol
       winner(player)

    elsif
       board[1][0] == symbol && 
       board[1][1] == symbol &&
       board[1][2] == symbol
       winner(player)

    elsif
       board[2][0] == symbol && 
       board[2][1] == symbol &&
       board[2][2] == symbol
       winner(player)

    elsif
       board[0][1] == symbol && 
       board[1][1] == symbol &&
       board[2][1] == symbol
       winner(player)

    elsif
       board[0][2] == symbol && 
       board[1][2] == symbol &&
       board[2][2] == symbol
       winner(player) 

    elsif
       board[0][2] == symbol && 
       board[1][1] == symbol &&
       board[2][0] == symbol
       winner(player) 

    elsif
       board[0][0] == symbol && 
       board[1][1] == symbol &&
       board[2][2] == symbol
       winner(player)

    end
     return false
end

If at least one (any?) of the triplets are equal to the given symbol, a player wins otherwise false.

def check_win?
     triplets =  [ [[0,0], [0,1], [0,2]] ... ]
     triplets.any? {|t| t.all? {|x, y| @board[x][y] == @symbol}} ? winner(player) : false
end

Actually I would have chosen a bool as return value, not winner_player

About move_left?

 def move_left?
     sum  = @track_move.inject{|r, s| r + s}
     if sum == 36
       return true
     end
end
  • sum is unecessary as an intermediate variable.
  • inject can be written more shortly.
  • if is not needed, nor return is

:

 def move_left?
     @track_move.inject(:+) == 36
 end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.