This 2-player tic-tac-toe game is the first thing I've really written from scratch in Ruby, without the guidance of a tutorial. I'm interested in knowing what could be improved upon to make the code better.
Please point out anything you see that deviates from common Ruby conventions, or that would improve the code in any way.
class Board
def initialize
@board = Array.new(3) { Array.new(3, " ") }
end
def printInstructions
puts "1 | 2 | 3",
"---------",
"4 | 5 | 6",
"---------",
"7 | 8 | 9"
print "\n"
end
def printBoard
(0..2).each do |row|
print " "
(0..2).each do |col|
print @board[row][col]
print " | " unless col == 2
end
print "\n"
print " ---------\n" unless row == 2
end
print "\n"
end
def findWinner
# X X X X
# & X
# X
(0..2).each do |i|
if @board[i][0] == @board[i][1] && @board[i][1] == @board[i][2]
return @board[i][0] unless @board[i][0] == " "
elsif @board[0][i] == @board[1][i] && @board[1][i] == @board[2][i]
return @board[0][i] unless @board[0][i] == " "
end
end
# X X
# X & X
# X X
if ( @board[0][0] == @board[1][1] && @board[1][1] == @board[2][2] ) ||
( @board[0][2] == @board[1][1] && @board[1][1] == @board[2][0] )
return @board[1][1] unless @board[1][1] == " "
end
# X O X
# X O X
# O X O
return "C" unless @board.join.split('').include?(" ")
# Undecided
return "U"
end
def dropPiece(piece, row, col)
@board[row][col] = piece if (0..2) === row &&
(0..2) === col &&
@board[row][col] == " "
end
end
board = Board.new
active_player = "X"
puts "\n" * 100
board.printInstructions
while board.findWinner == "U"
puts " #{active_player}'s turn. Choose a box!",
" **~V~**"
print " "
move = gets.chomp.to_i - 1
row = move / 3
col = move % 3
puts "\n" * 100
if board.dropPiece(active_player, row, col)
if active_player == "X"
active_player = "O"
else
active_player = "X"
end
else
puts " Invalid move, please select again\n\n"
end
board.printBoard
end
winner = board.findWinner
puts "\n" * 100
puts " --**~^^^^^^^~**--"
if winner == "C"
puts " C A T S G A M E"
else
puts " #{winner} ' S W I N"
end
puts " ----**~vVv~**----"
puts "\n"
board.printBoard
puts "\n **~V~**"
4 Answers 4
I suggest to divide the dropPiece
function in smaller, easy and readable parts as follows:
def is_empty(row,col)
@board[row][col] === " "
end
def inside_board(row,col)
(0..2) === row and (0..2) === col
end
def valid_move(row,col)
is_empty(row,col) and inside_board(row,col)
end
def dropPiece(piece, row, col) #makeMove
@board[row][col] = piece if valid_move(row,col)
end
The following piece of code should be
- Commented or
- A function with a meaningful name:
:
@board.join.split('').include?(" ")
I would have used:
def isTie
@board.join.split('').include?(" ")
end
return "C" if isTie
Do not be scared of writing small one-liner functions, they will increibly lower debugging time by improving modularity and readibility.
You use the so much puts "\n" * 100
define a function and use that:
def clearScreen
puts "\n" * 100
end
In findWinner
you should return false if you have not decided, this allows you to change the code from
while board.findWinner == "U"
to
while not board.findWinner
that is much clearer.
The following should be a function called something like alert_winner(winner)
puts "\n" * 100
puts " --**~^^^^^^^~**--"
if winner == "C"
puts " C A T S G A M E"
else
puts " #{winner} ' S W I N"
end
puts " ----**~vVv~**----"
puts "\n"
board.printBoard
puts "\n **~V~**"
The following should be a function called promptUser
puts " #{active_player}'s turn. Choose a box!",
" **~V~**"
print " "
move = gets.chomp.to_i - 1
row = move / 3
col = move % 3
In my opinion the main part should be the shorter possible and only call other functions
Minor: you should use and
instead of &&
, they are exactly the same but the first one is clearer.
Putting it all together, here's the improved version(I added a tiny method at the end):
class Board
def initialize
@board = Array.new(3) { Array.new(3, " ") }
end
def printInstructions
puts "1 | 2 | 3",
"---------",
"4 | 5 | 6",
"---------",
"7 | 8 | 9"
print "\n"
end
def printBoard
(0..2).each do |row|
print " "
(0..2).each do |col|
print @board[row][col]
print " | " unless col == 2
end
print "\n"
print " ---------\n" unless row == 2
end
print "\n"
end
def isTie
@board.join.split('').include?(" ")
end
def findWinner
# X X X X
# & X
# X
(0..2).each do |i|
if @board[i][0] == @board[i][1] && @board[i][1] == @board[i][2]
return @board[i][0] unless @board[i][0] == " "
elsif @board[0][i] == @board[1][i] && @board[1][i] == @board[2][i]
return @board[0][i] unless @board[0][i] == " "
end
end
# X X
# X & X
# X X
if ( @board[0][0] == @board[1][1] && @board[1][1] == @board[2][2] ) ||
( @board[0][2] == @board[1][1] && @board[1][1] == @board[2][0] )
return @board[1][1] unless @board[1][1] == " "
end
# X O X
# X O X
# O X O
return "C" unless isTie
return false
end
def is_empty(row,col)
@board[row][col] === " "
end
def inside_board(row,col)
(0..2) === row and (0..2) === col
end
def valid_move(row,col)
is_empty(row,col) and inside_board(row,col)
end
def dropPiece(piece, row, col)
@board[row][col] = piece if valid_move(row,col)
end
end
def clear_screen
puts "\n" * 100
end
def prompt_move(active_player)
puts " #{active_player}'s turn. Choose a box!",
" **~V~**"
print " "
move = gets.chomp.to_i - 1
row = move / 3
col = move % 3
return row,col
end
def alert_winner(winner,board)
puts " --**~^^^^^^^~**--"
if winner == "C"
puts " C A T S G A M E"
else
puts " #{winner} ' S W I N"
end
puts " ----**~vVv~**----"
puts "\n"
board.printBoard
puts "\n **~V~**"
end
def ticTacToe(boardClass)
board = boardClass.new
active_player = "X"
clear_screen
board.printInstructions
while not board.findWinner
row,col = prompt_move(active_player)
clear_screen
if board.dropPiece(active_player, row, col)
if active_player == "X"
active_player = "O"
else
active_player = "X"
end
else
puts " Invalid move, please select again\n\n"
end
board.printBoard
end
winner = board.findWinner
clear_screen
alert_winner(winner,board)
end
while true
clear_screen
puts "Do you want to play again? (y/n)"
if ["no","n"].include? (gets.chomp.downcase)
puts "Goodbye"
break
end
ticTacToe(Board)
end
Considering that the number pads on computer keyboards are arranged as
$$\begin{array}{|c|c|c|} \hline 7 & 8 & 9 \\ \hline 4 & 5 & 6 \\ \hline 1 & 2 & 3 \\ \hline \end{array}$$
I would suggest that inverting the grid numbering convention would produce a more pleasant user experience.
2 is an inexplicable magic number for tic-tac-toe. On the other hand, 3 is perfectly expected to appear.
Therefore, instead of writing your ranges as (0..2)
, you should write (0...3)
, using the three-dot form to exclude the upper bound.
-
\$\begingroup\$ ...What? That would make the size of the grid 4x4. Did you mean (1..3)? \$\endgroup\$Devon Parsons– Devon Parsons2015年01月12日 18:50:48 +00:00Commented Jan 12, 2015 at 18:50
-
\$\begingroup\$ @DevonParsons I meant what I wrote.
(0...3).to_a
produces[0, 1, 2]
. \$\endgroup\$200_success– 200_success2015年01月12日 18:59:41 +00:00Commented Jan 12, 2015 at 18:59 -
\$\begingroup\$ Ah, I failed to notice the triple period. \$\endgroup\$Devon Parsons– Devon Parsons2015年01月12日 20:03:19 +00:00Commented Jan 12, 2015 at 20:03
A quick note: examine these lines in your code.
if active_player == "X"
active_player = "O"
else
active_player = "X"
end
This is more succinctly expressed in a ternary, i.e:
active_player = active_player == "X" ? "O" : "X"
That's a direct translation and a much more 'ruby' way of doing it. There's another way to do it as well, but it's not necessarily better. It lends itself nicely to extending the number of players - imagine you later wanted a 4 player tic-tac-toe game.
def next_player(current_player)
players = ["X", "O", "T", "S"]
players[1+players.index(current_player)] || players[0]
end
I wouldn't use this with your current requirements, but the possibility is there if you needed the extensibility.
snake_case
to name methods \$\endgroup\$