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!
1 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.