1
\$\begingroup\$

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 location\n"
    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  
```
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

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 location\n"
    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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.