Problem
As a preface, I’m very new to coding (self-learning) so if you notice any bad habits please let me know, it would be GREATLY appreciated.
I’m self-learning Ruby so I don’t really have a frame of reference of if my coding is appropriate, easy to read, difficult to read etc… so I’m thankful for any feedback you guys have to help me improve in that regard but I’m grateful for any critique to help me improve my code and improve as a programmer.
This is my first Ruby project with more than one class, it’s a two player Tic-Tac-Toe game and it’s completely functional right now. I’m curious if I divided my classes up appropriately. I made a Game class that handles the actual gameplay itself, a Players class to handle the player instances, and a Board class that handles the gameboard display and updates.
repli link:
https://repl.it/@MaBuCode/LongMonthlyBsddaemon#main.rb
code:
class Game
WINNING_COMBOS = [
[0,1,2],
[3,4,5],
[6,7,8],
[0,3,6],
[1,4,7],
[2,5,8],
[0,4,8],
[6,4,2],
]
def initialize
@players = Players.new
end
#move method handles the actual gameplay
def move
@end = false
@gameboard = Board.new
@turn = 1
while @turn<10
if @turn % 2 != 0
turn_sequence(@players.player1, "X")
elsif @turn % 2 == 0
turn_sequence(@players.player2, "O")
end
end
end
#turn_sequence method handles position choice and switching player turns
def turn_sequence (player, symbol)
puts "#{player}(#{symbol}) please choose a position"
@player_move = gets.chomp.to_i
if (0..8).include?(@player_move) && @gameboard.board[@player_move] == " " && @end == false
@turn += 1
@gameboard.board_update(@player_move, symbol)
win_check
draw_check
else
puts "Please enter a number between 0 to 8 in an untaken locationn"
end
end
#method to check board array vs the winning combinations to check if there is a winner
def win_check
WINNING_COMBOS.each do |win_check|
if (@gameboard.board[win_check[0]] == @gameboard.board[win_check[1]] &&
@gameboard.board[win_check[1]] == @gameboard.board[win_check[2]]) &&
@gameboard.board[win_check[0]] != " "
if @gameboard.board[win_check[0]] == "X"
puts "#{@players.player1} WINS"
@turn = 10
@end = true
play_again?
elsif @gameboard.board[win_check[0]] == "O"
puts "#{@players.player2} WINS"
@turn = 10
@end = true
play_again?
end
end
end
end
#method to determine if all the positions are filled without a victory
def draw_check
if @turn == 10 && @end == false
puts "It's a draw"
play_again?
end
end
#method to ask the player if they wish to play again
def play_again?
puts "Play again? (Y/N)"
response = ""
while response != "Y" || response != "N"
response = gets.chomp.upcase
if response == "Y"
newgame = Game.new
newgame.move
elsif response == "N"
else
puts "Please enter (Y/N)"
end
end
end
end
#class which handles player name request and initialization of player instance variables
class Players
attr_reader :player1, :player2
def initialize
puts "Player 1, please enter your name"
@player1 = gets.chomp
puts "#{@player1} is X"
puts "Player 2, please enter your name"
@player2 = gets.chomp
puts "#{@player2} is O"
end
end
#class to handle everything to do with displaying the board, updating the board, and creating the board array itself
class Board
attr_reader :board
def initialize
puts "On your turn enter one of the following numbers to place your piece in the corresponding location:"
puts "0 | 1 | 2"
puts "---------"
puts "3 | 4 | 5"
puts "---------"
puts "6 | 7 | 8"
@board = [" "," "," "," "," "," "," "," "," "]
end
#method to update the @board array with "X" or "O"
def board_update(position, symbol)
@board[position] = symbol
game_board_display(@board)
end
#method that displays the updated board after each turn
def game_board_display (board)
puts "#{board[0]} | #{board[1]} | #{board[2]}"
puts "---------"
puts "#{board[3]} | #{board[4]} | #{board[5]}"
puts "---------"
puts "#{board[6]} | #{board[7]} | #{board[8]}"
end
end
game = Game.new
game.move
```
Solution
Consistency
Sometimes you are using 1 space for indentation, sometimes you are using 2, sometimes 4, sometimes none. Sometimes you are using whitespace around operators, sometimes you don’t, sometimes you are using whitespace on one side of the operator, but not the other. Sometimes, you use space after a comma, sometimes you don’t. Sometimes, you have a trailing comma after the last element of an array, sometimes you don’t. Sometimes you are using whitespace before the parameter list, sometimes you don’t.
You should choose one style and stick with it. If you are editing some existing code, you should adapt your style to be the same as the existing code. If you are part of a team, you should adapt your style to match the rest of the team.
Most communities have developed standardized community style guides. In Ruby, there are multiple such style guides. They all agree on the basics (e.g. indentation is 2 spaces), but they might disagree on more specific points (single quotes or double quotes).
In general, if you use two different ways to write the exact same thing, the reader will think that you want to convey a message with that. So, you should only use two different ways of writing the same thing IFF you actually want to convey some extra information.
For example, some people always use parentheses for defining and calling purely functional side-effect free methods, and never use parentheses for defining and calling impure methods. That is a good reason to use two different styles (parentheses and no parentheses) for doing the same thing (defining methods).
Indentation
The standard indentation style in Ruby is two spaces. You mostly use 2 spaces, but there are some places where you use 1, 4, or none. Stick with two.
Whitespace around operators
There should be 1 space either side of an operator.
Space after comma
There should be 1 space after a comma. You sometimes use 1 space, sometimes no space.
Trailing comma
In general, it is un-idiomatic in Ruby to have a trailing comma at the end of an array literal.
The argument for having trailing commas is basically that if you have one element per line and you always have trailing commas, then adding or removing one element will only add or remove that one line and thus lead to cleaner commits, smaller diffs, and easier merges. Personally, I do not buy into this argument.
Frozen string literals
Immutable data structures and purely functional code are always preferred, unless mutability and side-effects are required for clarity or performance. In Ruby, strings are always mutable, but there is a magic comment you can add to your files (also available as a command-line option for the Ruby engine), which will automatically make all literal strings immutable:
# frozen_string_literal: true
It is generally preferred to add this comment to all your files.
Object#freeze
Object#freeze is a method that allows you to freeze an object. A frozen object will no longer allow itself to be modified. It is good practice in general to freeze objects that you don’t intend to modify, both as a signal to the reader that this object will not be modified, and as a safety net, in case you ever accidentally try to modify it regardless.
We already made all but one of our strings frozen, so let’s do that with the arrays as well:
WINNING_COMBOS = [
[0, 1, 2],
[3, 4, 5],
[6, 7, 8],
[0, 3, 6],
[1, 4, 7],
[2, 5, 8],
[0, 4, 8],
[6, 4, 2]
].freeze
Numbers are immutable anyway, no need to freeze them.
Numeric predicates
@turn.odd?
reads more fluently than
@turn % 2 != 0
and the same for Integer#even?
No space before parameter list
In a method definition, there should be no whitespace between the method name and the parameter list, e.g. this:
def game_board_display (board)
should be
def game_board_display(board)
This is one of those cases I mentioned at the beginning: sometimes you use whitespace, sometimes not, but there doesn’t seem to be any rhyme or reason to when you choose one and when you choose the other. However, being programmers, when we read your code, we will search for this reason, and then we will get confused when we can’t find any, and we will assume that we don’t understand your code because we cannot figure out why you are doing that.
Linting
You should run some sort of linter or static analyzer on your code. Rubocop is a popular one, but there are others.
Rubocop was able to detect all of the style violations I pointed out above, and also was able to autocorrect all of them.
Let me repeat that: I have just spent two pages pointing out how to correct tons of stuff that you can actually correct within milliseconds at the push of a button. I have set up my editor such that it automatically runs Rubocop with auto-fix as soon as I hit “save”.
In particular, running Rubocop on your code, it detects 157 offenses, of which it can automatically correct 152. This leaves you with 5 offenses, of which 2 are very simple.
Here’s what the result of the auto-fix looks like:
# frozen_string_literal: true
class Game
WINNING_COMBOS = [
[0, 1, 2],
[3, 4, 5],
[6, 7, 8],
[0, 3, 6],
[1, 4, 7],
[2, 5, 8],
[0, 4, 8],
[6, 4, 2]
].freeze
def initialize
@players = Players.new
end
# move method handles the actual gameplay
def move
@end = false
@gameboard = Board.new
@turn = 1
while @turn < 10
if @turn.odd?
turn_sequence(@players.player1, 'X')
elsif @turn.even?
turn_sequence(@players.player2, 'O')
end
end
end
# turn_sequence method handles position choice and switching player turns
def turn_sequence(player, symbol)
puts "#{player}(#{symbol}) please choose a position"
@player_move = gets.chomp.to_i
if (0..8).include?(@player_move) && @gameboard.board[@player_move] == ' ' && @end == false
@turn += 1
@gameboard.board_update(@player_move, symbol)
win_check
draw_check
else
puts "Please enter a number between 0 to 8 in an untaken locationn"
end
end
# method to check board array vs the winning combinations to check if there is a winner
def win_check
WINNING_COMBOS.each do |win_check|
if (@gameboard.board[win_check[0]] == @gameboard.board[win_check[1]] &&
@gameboard.board[win_check[1]] == @gameboard.board[win_check[2]]) &&
@gameboard.board[win_check[0]] != ' '
if @gameboard.board[win_check[0]] == 'X'
puts "#{@players.player1} WINS"
@turn = 10
@end = true
play_again?
elsif @gameboard.board[win_check[0]] == 'O'
puts "#{@players.player2} WINS"
@turn = 10
@end = true
play_again?
end
end
end
end
# method to determine if all the positions are filled without a victory
def draw_check
if @turn == 10 && @end == false
puts "It's a draw"
play_again?
end
end
# method to ask the player if they wish to play again
def play_again?
puts 'Play again? (Y/N)'
response = ''
while response != 'Y' || response != 'N'
response = gets.chomp.upcase
if response == 'Y'
newgame = Game.new
newgame.move
elsif response == 'N'
else
puts 'Please enter (Y/N)'
end
end
end
end
# class which handles player name request and initialization of player instance variables
class Players
attr_reader :player1, :player2
def initialize
puts 'Player 1, please enter your name'
@player1 = gets.chomp
puts "#{@player1} is X"
puts 'Player 2, please enter your name'
@player2 = gets.chomp
puts "#{@player2} is O"
end
end
# class to handle everything to do with displaying the board, updating the board, and creating the board array itself
class Board
attr_reader :board
def initialize
puts 'On your turn enter one of the following numbers to place your piece in the corresponding location:'
puts '0 | 1 | 2'
puts '---------'
puts '3 | 4 | 5'
puts '---------'
puts '6 | 7 | 8'
@board = [' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ']
end
# method to update the @board array with "X" or "O"
def board_update(position, symbol)
@board[position] = symbol
game_board_display(@board)
end
# method that displays the updated board after each turn
def game_board_display(board)
puts "#{board[0]} | #{board[1]} | #{board[2]}"
puts '---------'
puts "#{board[3]} | #{board[4]} | #{board[5]}"
puts '---------'
puts "#{board[6]} | #{board[7]} | #{board[8]}"
end
end
game = Game.new
game.move
And here are the offenses that Rubocop could not automatically correct:
Offenses:
game.rb:3:1: C: Style/Documentation: Missing top-level class documentation comment.
class Game
^^^^^
game.rb:48:3: C: Metrics/AbcSize: Assignment Branch Condition size for win_check is too high. [<5, 28, 11> 30.5/17]
def win_check ...
^^^^^^^^^^^^^
game.rb:48:3: C: Metrics/MethodLength: Method has too many lines. [17/10]
def win_check ...
^^^^^^^^^^^^^
game.rb:70:5: C: Style/GuardClause: Use a guard clause (return unless @turn == 10 && @end == false) instead of wrapping the code inside a conditional expression.
if @turn == 10 && @end == false
^^
game.rb:77:3: C: Metrics/MethodLength: Method has too many lines. [12/10]
def play_again? ...
^^^^^^^^^^^^^^^
1 file inspected, 5 offenses detected
Let’s look at the simple one first.
Guard clauses
If you have a case where an entire method or block is wrapped in a conditional, you can replace that with a “guard clause” and reduce the level of nesting.
E.g. this:
def something
if foo
bar
baz
quux
else
42
end
end
can become this:
def something
return 42 unless foo
bar
baz
quux
end
There are a couple of opportunities to do this in your code, and a couple more are created by following the Rubocop advice.
So, your draw_check method:
def draw_check
if @turn == 10 && @end == false
puts "It's a draw"
play_again?
end
end
can become this:
def draw_check
return unless @turn == 10 && @end == false
puts "It's a draw"
play_again?
end
Redundant checks
There are some redundant checks in your code, for example here:
if @turn.odd?
turn_sequence(@players.player1, 'X')
elsif @turn.even?
turn_sequence(@players.player2, 'O')
end
There is no need to check for even?. If the number is not odd, it is even, it’s not like there is a third choice. This should just be an else instead of an elsif.
Equality with booleans
@end == false
@end is already a boolean, there is no need to check for equality to false. This is just
!@end
Unnecessary instance variables
The instance variable @player_move is only ever used in one method. It should be a local variable instead.
Redundant operations
String#to_i ignores whitespace, so there is no need to chomp the whitespace before parsing the string as an integer.
Array#include? over multiple comparisons
If you have a pattern like this:
while response != 'Y' || response != 'N'
It is more idiomatic to use
until %w[Y N].include?(response)
Naming
Question marks in method names are usually reserved for predicate methods, i.e. for methods that are pure and return a boolean-ish value. Your play_again? method has a lot of side-effects and doesn’t return anything, so it should not have a question mark.
The Elephant in the room
One thing I have not addressed so far, and that I unfortunately do not have to time to address, is the fundamental design of the code. Everything I mentioned so far is just cosmetics.
You are mixing I/O and logic everywhere. A method should either print something or do something. Your design makes it impossible to test the code without actually playing the game. I cannot prepare a file with moves and feed it to a test runner, I actually have to manually play the game.
Your Players class is violating the Zero-One-Many Rule, which states that there should only ever be none of a thing, one of a thing, or many of a thing. Having a single object that represents exactly two players is strange. It would make much more sense to have a Player class and have two instances of it.