3
\$\begingroup\$

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~**"
Malachi
29k11 gold badges86 silver badges188 bronze badges
asked Jan 8, 2015 at 11:23
\$\endgroup\$
2
  • \$\begingroup\$ It's been a year since you finished your first complete ruby program! Here is a challenge: rewrite it for an arbitrary number of players in an arbitrarily-sized board. :) \$\endgroup\$ Commented Jan 12, 2015 at 20:34
  • \$\begingroup\$ You should read the ruby style guide github.com/bbatsov/ruby-style-guide. A number of things stand out, most glaring is that in ruby we use snake_case to name methods \$\endgroup\$ Commented Jan 13, 2015 at 13:10

4 Answers 4

2
\$\begingroup\$

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

  1. Commented or
  2. 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
answered Jan 8, 2015 at 13:53
\$\endgroup\$
2
\$\begingroup\$

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.

answered Jan 8, 2015 at 12:56
\$\endgroup\$
1
\$\begingroup\$

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.

answered Jan 8, 2015 at 12:58
\$\endgroup\$
3
  • \$\begingroup\$ ...What? That would make the size of the grid 4x4. Did you mean (1..3)? \$\endgroup\$ Commented Jan 12, 2015 at 18:50
  • \$\begingroup\$ @DevonParsons I meant what I wrote. (0...3).to_a produces [0, 1, 2]. \$\endgroup\$ Commented Jan 12, 2015 at 18:59
  • \$\begingroup\$ Ah, I failed to notice the triple period. \$\endgroup\$ Commented Jan 12, 2015 at 20:03
0
\$\begingroup\$

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.

answered Jan 14, 2015 at 13:35
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.