Tic Tac Toe in Ruby

Question

I'm a novice programmer going through the Odin Project. I just finished up building a command line Tic Tac Toe game and I would appreciate some feedback on object-oriented design, methods, verbosity, etc.

Here is the code for the main menu/high level game control:

require "./game.rb"
require "./player.rb"
require "./board.rb"

module TicTacToe
  class TicTacToeRunner
    attr_accessor :name1, :name2, :game_data

    def initialize
      welcome_screen
      game_loop
    end

    def welcome_screen #greets players, asks for names
      puts "|| Welcome to Tic Tac Toe! ||"
      puts "||-------------------------||\n\n\n"
      print "Enter Player 1's name: "
      @name1 = gets.chomp
      puts " "
      print "Enter Player 2's name: "
      @name2 = gets.chomp
      puts " "
    end

    def game_loop #loops between running the game and asking if user wants to play again
      game_start
      play_again
    end

    def game_start #creates and runs a game
      board = Board.new
      player1, player2 = Player.new(@name1), Player.new(@name2)
      game = Game.new(player1, player2, board)
    end


    def play_again #asks user if they want to play again
      input = nil
      until input == "Y" or input == "N"
        puts "Would you like to play again? (Y/N): "
        input = gets.chomp.upcase
      end
      case input
      when "Y"
        game_start
      when "N"
        puts "Thank you for playing!"
      end
    end
  end
end

TicTacToeRunner.new

Here is one game of tic-tac-toe (controls players and the board):

require "./board.rb"
require "./player.rb"

module TicTacToe
  class Game
    attr_accessor :player1, :player2, :turn, :board

    @@winning_positions = [
      [0, 1, 2], [3, 4, 5], [6, 7, 8],
      [0, 3, 6], [1, 4, 7], [2, 5, 8],
      [0, 4, 8], [2, 4, 6]
    ]

    def initialize(player1, player2, board)
      ##initialize external objects utilized by this class.
      @player1 = player1
      @player2 = player2
      @board = board

      ##initialize data for current game run
      @current_turn = 1
      @first_turn = ""
      @winner = ""

      #executes game flow
      play
    end

    def play #main flow of game
      pick_first_turn
      allocate_symbols
      take_turns
    end

    private

    def pick_first_turn #a player is randomly chosen to go first
      random = Random.new
      first_turn = random.rand(0..1)
      case first_turn
      when 0
        @first_turn = @player1.name
      when 1
        @first_turn = @player2.name
      end
      puts "#{@first_turn} goes first!\n\n\n\n"
    end

    def allocate_symbols #allocates the symbols to the players
      @player1.sym = "X"
      @player2.sym = "O"
    end

    def take_turns #take turns(loops) between the players depending on who started first and the current turn number
      until draw? || @winner != ""
        if @first_turn == @player1.name
          (@current_turn.even?) ? turn(@player2) : turn(@player1)
        elsif @first_turn == @player2.name
          (@current_turn.even?) ? turn(@player1) : turn(@player2)
        end
      end
      puts "Game was a draw!" if draw? #checks if game is a draw after loop ends
    end

    def turn(player) #one turn for a player
      puts "Turn #{@current_turn}:"
      puts "---------------------------\n\n\n"
      @board.generate_board
      @board.add_symbol(get_valid_position(player), player.sym)
      announce_win(player)
      @current_turn += 1
    end

    def get_valid_position(player) #gets valid input from player.
      input = 0
      until valid_input?(input)
        print "#{player.name}, enter the cell number that you would like to use (1-9): "
        input = gets.chomp.to_i
        print "Invalid input! " unless valid_input?(input)
        puts "Number is not in range 1-9" unless (input > 0 && input < 10)
        puts "Cell taken." if @board.space_taken?(input - 1)
      end
      input - 1
    end

    def announce_win(player)
      check_winner(player)
      if @winner == player.name
        puts "#{player.name} is the winner!"
      end
    end

    def draw? #checks if the game is a draw
      (@current_turn == @board.spaces.length) && (@winner == "")
    end

    def check_winner(player) #if a player is a winner, the @winner instance var is set to that player
      @@winning_positions.each do |triplet|
        @winner = player.name if triplet.all? { |a| @board.spaces[a] == player.sym }
      end
    end

    def valid_input?(input) #checks if input meets conditions
      return input > 0 && input < 10 && [email protected]_taken?(input - 1)
    end
  end
end

Here is the player class:

module TicTacToe
  class Player
    attr_accessor :name, :sym 
    def initialize(name)
      @name = name
    end
  end
end

Here is the Board class:

module TicTacToe
  class Board
    attr_accessor :spaces

    def initialize
      @spaces = Array.new(9, "-")
    end

    def generate_board 
      board = ""
      (0..8).step(3) do |i|
        board += "| #{@spaces[i]} | #{@spaces[i + 1]} | #{@spaces[i + 2]} |\n"
      end
      puts board + "\n\n\n"
    end

    def add_symbol(position, symbol) #adds a symbol to a certain position
      @spaces[position] = symbol
    end

    def space_taken?(position) #checks if a certain space is taken
      return @spaces[position] == "X" || @spaces[position] == "O"
    end
  end
end

Thanks a ton in advance!

Answer 1

Your code is clear, well-formatted and readable most of the time. It's very nice that you pay attention to that. However, sometimes you design your code in a weird way that makes it unnecessarily hard to follow and maintain. That's OK though, this fixes only with experience.

Some comments in your code are superfluous. For example, you don't need to state that your initialize method initializes stuff: that's obvious. You also don't need to state that allocate_symbols allocates symbols - you picked a good name for your method, which, again, makes it obvious, so you don't need the comment. if draw? doesn't need a comment "check for draw" - it's obvious. It's called self-documenting code, which is sought-after quality, especially in Ruby.

The way you did your play_again and game_loop will increase stack depth every time a new game will start, and also makes it a bit hard to follow. It would be better to apply single responsibility principle, and have your play_again return true/false depending on player choice, and let game_loop handle looping:

def play_again
  loop do # notice much simpler loop
    input = gets.chomp.upcase
    if input == "Y" 
      return true
    elsif input == "N"
      return false 
    end
  end
end

def game_loop
  while play_again
    game_start
  end
end

Also, there is awesome method called sample which makes this very easy:

def pick_first_turn #a player is randomly chosen to go first
  @first_turn = [@player1.name, @player2.name].sample
  puts "#{@first_turn} goes first!\n\n\n\n"
end

Your turn logic would be much easier if you remembered which player has the current turn and alternated that, instead of holding the first one and calculating this each time. For example:

def next_turn
  @current_turn_player == @player1 ? @player2 : @player1
  @turn_count +=1
end

This would, of course, change the rest of the logic a bit, but for the better as well:

def take_turns 
  until draw? || @winner != ""
    next_turn
    turn(@current_turn_player)
  end
  puts "Game was a draw!" if draw?
end

Complex conditions are nice to put in their own methods:

def game_end?
  draw? || @winner != ""
end

This makes code that uses them more readable, and improves self-documentation of the code. This condition would probably need a comment, but again, a nice name makes it unnecessary. While we at it, you probably would do better if @winner was initialized with nil instead of empty string.