patternrubyMinor

Plays a game of Tic Tac Toe

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

toeplaystictacgame

Problem

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 #{

Solution

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

Code Snippets

# 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
      diag

Context

StackExchange Code Review Q#86129, answer score: 4

Revisions (0)

No revisions yet.