4
\$\begingroup\$

I have done Tictactoe game in Ruby. It is my first Object Oriented project in Ruby. I would like to separate all program into a few classes such as Player Board and Game. I want to use more OOP best practices.
Also, my move method is looking so big. I would like to refactor my move method because the code is repeating. In a game class, there is 2 break if code piece. Is there any way to reduce it? Lastly, In my module class, do you think that everything is okay or should I use a more concise way?
And also a total review would be great! Thank you for now.

Here is the code. It is working.

module CheckWinner
  def check_horizontal(player1, player2)
    player1_win = false
    player2_win = false
    @board.each do |row|
      player1_win = row.all?(player1)
      player2_win = row.all?(player2)
      break if player1_win || player2_win
    end
    puts "#{player1} won!" if player1_win
    puts "#{player2} won!" if player2_win
    player1_win || player2_win
  end

  def check_vertical(player1, player2)
    player1_win = false
    player2_win = false
    @board.transpose.each do |row|
      player1_win = row.all?(player1)
      player2_win = row.all?(player2)
      break if player1_win || player2_win
    end
    puts "#{player1} won!" if player1_win
    puts "#{player2} won!" if player2_win
    player1_win || player2_win
  end

  def check_diagonal(player1, player2)
    if @board[0][0] == player1 && board[1][1] == player1 && board[2][2] == player1 ||
       @board[0][2] == player1 && board[1][1] == player1 && board[2][0] == player1
      puts "#{@player1} won!"
      true

    elsif @board[0][0] == player2 && board[1][1] == player2 && board[2][2] == player2 ||
          @board[0][2] == player2 && board[1][1] == player2 && board[2][0] == player2
      puts "#{@player2} won!"
      true
    end
  end
end

# TicTacToe Board
class Board
  include CheckWinner
  attr_accessor :board

  def initialize
    @board = [[1, 2, 3], [4, 5, 6], [7, 8, 9]]
  end

  def print_board
    puts '-------------'
    @board.each do |row|
      print '|'
      row.each do |col|
        print " #{col}"
        print ' | '
      end
      puts
    end
    puts '-------------'
  end

  def occupied_error(value)
    puts 'There is a value or wrong place! Try Again!'
    print_board
    value == @player1 ? player2(@player1) : player1(@player2) # Stay same player
  end

  def move(place, value) # Place : 1-9, value = X || Y
    case place
    when 1
      if @board[0][0] == 'X' || @board[0][0] == 'O'
        occupied_error(value)
      else
        @board[0][0] = value
      end
    when 2
      if @board[0][1] == 'X' || @board[0][1] == 'O'
        occupied_error(value)
      else
        @board[0][1] = value
      end
    when 3
      if @board[0][2] == 'X' || @board[0][2] == 'O'
        occupied_error(value)
      else
        @board[0][2] = value
      end
    when 4
      if @board[1][0] == 'X' || @board[1][0] == 'O'
        occupied_error(value)
      else
        @board[1][0] = value
      end
    when 5
      if @board[1][1] == 'X' || @board[1][1] == 'O'
        occupied_error(value)
      else
        @board[1][1] = value
      end
    when 6
      if @board[1][2] == 'X' || @board[1][2] == 'O'
        occupied_error(value)
      else
        @board[1][2] = value
      end
    when 7
      if @board[2][0] == 'X' || @board[2][0] == 'O'
        occupied_error(value)
      else
        @board[2][0] = value
      end
    when 8
      if @board[2][1] == 'X' || @board[2][1] == 'O'
        occupied_error(value)
      else
        @board[2][1] = value
      end
    when 9
      if @board[2][2] == 'X' || @board[2][2] == 'O'
        occupied_error(value)
      else
        @board[2][2] = value
      end
    else
      puts 'It is not a valid value,please try again!(0..9)'
      occupied_error(value)
    end
  end

  def full?
    if @board.flatten.all?(String)
      puts "Draw!"
      true
    end
  end

  def choosing_player
    puts 'Choose for Player1(X or O)'
    loop do
      @player1 = gets.chomp!
      break if @player1 == 'X' || @player1 == 'O'

      puts 'Try Again!(X or O)'
    end
    puts "Player 1 is: #{@player1}"

    @player1 == "X" ? @player2 = "O" : @player2 = "X"
    puts "Player 2 is: #{@player2}"
    print_board
  end

  def player1(player1)
    puts "Choice #{player1} Place on a board(1 to 10)"
    @place = gets.chomp!.to_i
    move(@place, player1)
    print_board
  end

  def player2(player2)
    puts "Choice #{player2} Place on a board(1 to 10)"
    @place = gets.chomp!.to_i
    move(@place, player2)
    print_board
  end

  def game
    choosing_player
    loop do
      player1(@player1)
      break if check_vertical(@player1, @player2) == true || check_diagonal(@player1, @player2) == true || check_horizontal(@player1, @player2) == true || full?

      player2(@player2)
      break if check_vertical(@player1, @player2) == true || check_diagonal(@player1, @player2) == true || check_horizontal(@player1, @player2) == true || full?
    end
  end
end

board = Board.new
board.game

Repl it repo : https://replit.com/@Burakkepuc/TicTacToe#main.rb

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ Feb 25, 2022 at 10:38

2 Answers 2

3
\$\begingroup\$

Welcome to Code Review, you have almost the same code block repeated 9 times:

  if @board[0][2] == 'X' || @board[0][2] == 'O'
    occupied_error(value)
  else
    @board[0][2] = value
  end

So write a function and use it instead:

def assign_if_possible(x, y, value)
      if @board[y][x] == 'X' || @board[y][x] == 'O'
        occupied_error(value)
      else
        @board[y][x] = value
      end

and use it instead. Also using the % modulo operator should allow you to avoid the case statement altogether, something like (x = val % 3, y = (val / 3).to_i), the exact expression should not be too hard to find.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you I changed the code, but I hardcoded it in an ` assign_if_possible` parameters. Am I correct ? So I mean inside the move method ` when 1 assign_if_possible(0,0,value) ` \$\endgroup\$ Feb 3, 2022 at 16:40
  • \$\begingroup\$ @theplaceofburak yes, that is the simpler but longer possibility, otherwise you can use the % operator as said here to go from 1d coordinate to 2d coordinates to avoid the switch statement stackoverflow.com/questions/5494974/… \$\endgroup\$
    – Caridorc
    Feb 4, 2022 at 11:14
  • \$\begingroup\$ Thank you for your answer. \$\endgroup\$ Feb 4, 2022 at 13:23
1
\$\begingroup\$

Simplifying Board referencing will profoundly simplify the using code.

@board = [[1, 2, 3], [4, 5, 6], [7, 8, 9]]

#using actual coordinates
@diagonals = [
               [@board[0][0], @board[1][1], @board[2][2]],
               [@board[0][2], @board[1][1], @board[2][0]], 
             ]
@columns   = [
               [@board[0][0], @board[1][0], @board[2][0]],
               [@board[0][1], @board[1][1], @board[2][1]], 
               [@board[0][2], @board[1][2], @board[2][2]], 
             ]

@rows   =    [
               [@board[0][0], @board[0][1], @board[0][2]],
               [@board[1][0], @board[1][1], @board[1][2]], 
               [@board[2][0], @board[2][1], @board[2][2]], 
             ]

def winner? (player) {  # 'X' or 'O'
  won = false
  won = @[email protected]      { |row|   row.all?(player) ? true : won }
  won = @[email protected] { |diag| diag.all?(player) ? true : won }
  won = @[email protected]   { |col|   col.all?(player) ? true : won }

  won
end #winner?  

The program should accept lower case "x" or "o" and change it without making the user re-enter. And same consideration if user enters zero instead of oh. Or is it vice versa?


Use integers as Hash keys

Then mapping [[1, 2, 3], [4, 5, 6], [7, 8, 9]] to the board will subdue that awful case statement

@user_entry = {
            1 => @board[0][0],
            2 => @board[0][1],
            3 => @board[0][2]
            # et cetera
          }

Bonus! Now the board can be initialized with nil and still have user enter 1 - 9. A consistent initial value will simplify more code.

def move(player, entry) {
   @user_entry[entry] = player unless occupied?(@user_entry[entry])
end

def occupied?(this_square)
  return @user_entry[this_square].nil? ? false : true
end
\$\endgroup\$

Your Answer

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

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