5
\$\begingroup\$

As part of the odin project I've written this bit of code that allows two users to play a game of Tic Tac Toe. I'm still a relative newbie to Ruby and to coding and am guessing that there are much better ways to do what I've done here. I'd love to get input, suggestions and recommendations.

The full code is below but can also be found here.

class TicTacToe

    def initialize
        @game = Game.new("Chris", "Lisa")
    end

    class Game
        attr_accessor :player1, :player2

        def initialize (player1, player2)
            @player1 = Player.new(player1)
            @player2 = Player.new(player2)
            @winning = [[1,2,3], [4,5,6], [7,8,9], [1,4,7], [2,5,8], [3,6,9], [1,5,9], [3,5,7]]
            @symbol1 = "X"
            @symbol2 = "O"

            self.start
        end


        def start
            introduction
            player_names
            player_assignment
            display_board(@winning)
            play_game
        end

        def introduction
            puts "Welcome to TicTacToe. We will now randomly choose who goes first. Press any key to begin."
            puts ""
            @@input = gets.chomp
        end

        def player_names
            puts "Please enter the name of a player."
            name1 = gets.chomp
            puts ""
            puts "Please enter the name of another player."
            name2 = gets.chomp

            @player1.name = name1
            @player2.name = name2
        end

        def player_assignment
            @x = rand(10)   

            if @x <= 5
                @player1.symbol = @symbol1
                @player2.symbol = @symbol2
                puts ""
                puts "#{player1.name} is assigned #{@symbol1} and #{player2.name} is assigned #{@symbol2}"
                puts ""
            else
                @player1.symbol = @symbol2
                @player2.symbol = @symbol1
                puts ""
                puts "#{player1.name} is assigned #{@symbol2} and #{player2.name} is assigned #{@symbol1}"
                puts ""
            end
        end

        def play_game
            count = 0
            if @player1.goes_first? == true
                while count < 8 
                    check_board(@player1.play, @player1.symbol)
                    announce_winner(is_winner?(@player1))
                    count += 1

                    check_board(@player2.play, @player2.symbol) 
                    announce_winner(is_winner?(@player2))
                    count += 1
                end
            else
                while count < 8
                    check_board(@player2.play, @player2.symbol)
                    announce_winner(is_winner?(@player2))
                    count += 1

                    check_board(@player1.play, @player1.symbol)
                    announce_winner(is_winner?(@player1))
                    count += 1
                end
            end
            end_game
        end

        def display_board(array)
            puts ""
            print "  #{array[0][0]}  |   #{array[0][1]}  |   #{array[0][2]} \n"
            puts "------------------"
            print "  #{array[1][0]}  |   #{array[1][1]}  |   #{array[1][2]} \n"
            puts "------------------"
            print "  #{array[2][0]}  |   #{array[2][1]}  |   #{array[2][2]}"
            puts "\n"
        end

        def check_board(move, symbol)
            @move = move.to_i
            @symbol = symbol

            @winning.each do |array| 
                array.map! do |num| 
                    if num.is_a?(String)
                        num
                    elsif(@move == num)
                        @symbol
                    elsif @move != num && num.is_a?(Integer)
                        num         
                    end
                end
            end
            display_board(@winning)
            puts ""
        end

        def is_winner?(player)

            @winning.any? do |line|
                line.all? {|position| position == player.symbol}
            end
        end

        def announce_winner(x)
            @x = x

            if @x == true
                puts "You've won, congratulations"
                display_board(@winning)
                exit
            else
                return
            end
        end

        def end_game
            puts "GAME OVER"
            exit
        end
    end

    class Player
        attr_accessor :name, :symbol

        def initialize(name)
            @name = name
        end

        def play
            puts "#{@name}, your symbol is #{self.symbol}."
            puts "Please select a square represented."
            input = gets.chomp
            puts "#{@name} selected #{input} and a #{symbol} will be placed in that position."
            puts ""
            return input
        end

        def goes_first?
            if self.symbol == "X"
                return true
            else
                return false    
            end
        end
    end
end

a = TicTacToe.new
\$\endgroup\$
  • \$\begingroup\$ Not a real answer, but what will you do when the requirement or need arises to change the size of the board? say to 4x4 or 5x5? In that case you will have to make several changes to your code (displaying, winning conditions, etc.). \$\endgroup\$ – David Perfors Apr 8 '15 at 10:34
4
\$\begingroup\$

As Caridorc points out, the code is readable overall. I do however have some concerns:

  • Typically, Ruby style guides will say that you should use 2 spaces for indentation

  • Why is the game initialized with "Chris" and "Lisa" as players, when it never uses that for anything? Given that the game asks for players anyway, it shouldn't be necessary to pass anything to Game.new.

  • I'd suggest making TicTacToe a module instead, just for namespacing purposes. There's little need for it to be a class.

  • Your method names are a little off.

    • #announce_winner doesn't necessarily announce a winner
    • #check_board doesn't check the board
    • #is_winner? should probably just be #winner? ("is_" isn't necessary when you've got the question mark)
    • #player_names doesn't return player names, as its name would suggest as it's a noun.
    • #player_assignment is also a noun, but should be a verb, e.g. shuffle players, since that's what it does.
  • I'm very confused as to why #introduction creates a class variable called @@input. There's no reason to store the user's input. Nor is there a reason to chomp it.
    ... and "press any key to begin" isn't quite true: You have to press enter.

  • Also of note are the @symbol1 and @symbol2 variables, which don't seem to have any reason to be instance variables in Game. In general, you seem to use instance variables when you should use local variables.

  • You don't need most - if not all - of the returns in the code.

  • As Caridorc points out, there's quite a few instances of code duplication. Often for dealing with either @player1 or @player2.

  • The #start method is very clear in making the steps of the game explicit, but the #play_game method is more muddled. You always call #announce_winner.. even if there's nothing to announce.

  • Speaking of #announce_winner: Why create an instance variable to store an argument variable? Why have an else branch that does nothing?

  • I wouldn't let the Player be responsible for getting input from the user. It splits the responsibility for interacting with the user between Game and Player Doubly so, since using that input for anything happens in Game.

  • As mentioned, you have a bunch of instance variables that shouldn't be instance variables, but for some reason, the actual state of the game - the board - is not an instance variable. Instead, you pass an array around from method to method. It's almost the opposite of how object-oriented programming usually works.

  • You can keep picking already-claimed squares... Probably not what you want.
    It doesn't change the board, but it makes you forfeit your turn. You can keep picking, say, 5 for both players, and after 9 turns, the game just ends, even though there are 8 unclaimed squares on the board.
    You can also just type any random thing other than 1-9, and the game will let you. Again, you just forfeit your turn.
    In short, you should really make sure that user's input makes sense before continuing with the game.


Here's a very different implementation (I did it mostly for fun). I'm not saying this is "the right way™" (there are things I'm not happy with, as you can see in the comments). It's just to present an alternative.

# A tiny mixin for Array
class Array
  # Get every nth element in the array
  def every_nth(step)
    0.step(self.count, step).map { |i| self[i] }
  end
end

module TicTacToe
  # Convenience method
  def self.play
    Game.new.play
  end

  class Game
    def initialize
      @board = Board.new
      show_welcome_message
      create_players
      take_turn until @board.full?
      puts "It's a tie!"
    end

    def show_welcome_message
      puts "Welcome to TicTacToe. Press enter to begin."
      gets
    end

    def create_players
      names = []
      print "Please enter the name of the first player: "
      names << gets.chomp # TODO: Should check that something was actually entered

      print "Please enter the name of the second player: "
      names << gets.chomp # TODO: See above...

      puts "Randomizing who'll start..."

      @players = names.shuffle.zip(["X", "O"]).map do |name, symbol|
        Player.new(symbol, name)
      end
    end

    def print_board
      puts @board
    end

    # Note: I'm not too happy with this method. It's a little messy and recursive...
    def take_turn
      print_board
      puts "It's #{current_player.name}'s (#{current_player.symbol}) turn."
      square = get_player_choice
      @board[square] = current_player.symbol
      check_board # this may exit

      puts ""
      @players.rotate! # swap players around for next round
    end

    def current_player
      @players.first
    end

    def get_player_choice
      begin
        print "Enter the number of an unclaimed square: "
        square = gets.chomp.to_i
      end until @board.unclaimed_squares.include?(square)
      square
    end

    def check_board
      if @board.won?(current_player.symbol)
        puts "#{current_player.name} wins!"
        exit
      end
    end
  end

  # The Player class is simply a Struct
  Player = Struct.new(:symbol, :name)

  # The Board class handles the state of play, checking win conditions etc.
  # I'm not quite happy with checking for Fixnums or Strings - a bit too informal...
  class Board
    attr_reader :cells

    def initialize
      @squares = (1..9).to_a
    end

    def won?(symbol)
      winning_squares.any? do |line|
        line.all? { |value| value == symbol }
      end
    end

    def full?
      @squares.all? { |square| square.is_a?(String) }
    end

    def []=(number, value)
      @squares[number-1] = value
    end

    def unclaimed_squares
      @squares.select { |square| square.is_a?(Fixnum) }
    end

    def to_s
      lines = rows.map do |row|
        row.join(" | ")
      end
      lines.join("\n#{'-' * 9}\n")
    end

    private

    def winning_squares
      rows + columns + diagonals
    end

    def rows
      @squares.each_slice(3).to_a
    end

    def columns
      rows.transpose
    end

    def diagonals
      diagonals = []
      diagonals << @squares.every_nth(4) # top-left to bottom-right
      diagonals << @squares.reverse.every_nth(4) # the other one
      diagonals
    end
  end
end

TicTacToe.play
\$\endgroup\$
0
\$\begingroup\$

I can simplify your player_assignment:

  • You used a random variable up to ten and then checked if it was less than 5, you can use a random boolean instead

  • You repeated very similar code two times, I removed that duplication

def player_assignment

    if rand(2) == 1
        @player1.symbol = @symbol1
        @player2.symbol = @symbol2
    else
        @player1.symbol = @symbol2
        @player2.symbol = @symbol1
    end

    puts ""
    puts "#{player1.name} is assigned #{@player1.symbol} and #{player2.name} is assigned #{@player2.symbol}"
    puts ""
end

Also as a simple tip do not overcheck things that are already booleans

def goes_first?
    if self.symbol == "X"
        return true
    else
        return false    
    end
end

should become:

def goes_first?
    return self.symbol == "X"
end

I am pretty sure that there is some duplication to be removed down here but I can't understand how:

    if @player1.goes_first? == true
        while count < 8 
            check_board(@player1.play, @player1.symbol)
            announce_winner(is_winner?(@player1))
            count += 1

            check_board(@player2.play, @player2.symbol) 
            announce_winner(is_winner?(@player2))
            count += 1
        end
    else
        while count < 8
            check_board(@player2.play, @player2.symbol)
            announce_winner(is_winner?(@player2))
            count += 1

            check_board(@player1.play, @player1.symbol)
            announce_winner(is_winner?(@player1))
            count += 1
        end
    end

You should not use explicit while loops with count,

while count < 8

should become

(1..9).each do |turns_played|

removing the increment.

Overall the code is very clearly written, kudos.

\$\endgroup\$
  • \$\begingroup\$ @rand_bool = rand(2); if @rand_bool... This does not work, rand(2) always returns a truthy value (0 or 1). You need to have a smarter if condition; try if rand(2) == 1 and get rid of @rand_bool entirely (which, by the way, shouldn't be an instance var) \$\endgroup\$ – Devon Parsons Apr 7 '15 at 19:09
  • \$\begingroup\$ @DevonParsons so 0 is truthy, I am always learning, thanks. \$\endgroup\$ – Caridorc Apr 7 '15 at 19:12
  • \$\begingroup\$ Yup, only nil and false are falsey - everything is truthy. By the way, the goes_first? method can be reduced to just symbol == "X". The return is implicit, and the self is necessary either :) \$\endgroup\$ – Flambino Apr 7 '15 at 21:24

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.