5
\$\begingroup\$

I would appreciate any feedback you could offer. I'm self-learning, so I don't really know a "standard" that my code should be up to. I'm also not sure how to get rid of that bit of logic at the bottom, or if it's acceptable.

class Board 
    attr_reader :board, :place

    def initialize
        @board = [1, 2, 3, 4, 5, 6, 7, 8, 9]
        puts "Player 1 is X, Player 2 is O."
        show_board
    end

    def show_board
        @board.each_slice(3) { |x| puts x.join }
    end

    def get_place
        @place = gets.to_i - 1
    end

    def free_space?
        @board[place].is_a? Integer
    end

    def put_marker(mark)
        get_place
        until free_space?
            puts "Invalid move, choose again!"
            get_place
        end
        @board[place] = mark
        show_board
    end

    def player1_win_condition
        ary = [[1, 2, 3], [4, 5, 6], [7, 8, 9], [1, 4, 7], [2, 5, 8], [3, 6, 9], [1, 5, 9], [3, 5, 7]]
        ary.any? do |array| 
            array.all? { |num| @board[num - 1] == "X" }
        end
    end

    def player2_win_condition
        ary = [[1, 2, 3], [4, 5, 6], [7, 8, 9], [1, 4, 7], [2, 5, 8], [3, 6, 9], [1, 5, 9], [3, 5, 7]]
        ary.any? do |array| 
            array.all? { |num| @board[num - 1] == "O" } 
        end
    end

    def draw_condition
        @board.all? { |num| num.is_a? String }
    end

    def end_game
        if player1_win_condition == true
            puts "Player 1 wins!"
            exit
        elsif player2_win_condition == true
            puts "Player 2 wins!" 
            exit
        elsif draw_condition == true && player1_win_condition == false && player2_win_condition == false
            puts "It's a draw!"
            exit
        end
    end
end

class Player
    attr_accessor :mark

    def initialize(mark)
        @mark = mark
    end
end

player_1 = Player.new("X")
player_2 = Player.new("O")
new_board = Board.new


until new_board.end_game
    puts "Player 1 turn"
    new_board.put_marker(player_1.mark)
    if new_board.end_game != true
        puts "Player 2 turn"
        new_board.put_marker(player_2.mark)
    end
end
\$\endgroup\$
3
\$\begingroup\$

Gratulation! For a beginner you made a good and clear game.

A first modified version:

class Board 
    attr_reader :board

    def initialize(player1, player2)
        @board = (1..9).to_a    #You can use also a range to initialize the array
        puts "Player 1 is %s, Player 2 is %s." % [player1.mark, player2.mark]
        @player_1 = player1
        @player_2 = player2
        show_board
    end

    def play

      until end_game?
          puts "Player 1 turn"
          put_marker(@player_1.mark)
          if ! end_game?
              puts "Player 2 turn"
              put_marker(@player_2.mark)
          end
        end
    end

    def show_board
        puts '-'*9          #I prefer some lines in the board
        @board.each_slice(3) { |x| #Good! You use slice.
            puts x.join(' | ') 
            puts '-'*9
          }
    end

    def get_place
      while
          place = gets.to_i - 1 #Use a return parameter
        #Check also if you get only values between 1 and 9 (no letters, no 0)
          if ! place.between?(0,9)
              puts "Invalid move, use 1..9. Please choose again!"
              next
          end
          if ! free_space?(place) 
              puts "Invalid move, Pos. %i is occupied. Please choose again!"
              next
            end
            break #stop loop, Value is ok
        end
      return place
    end

    def free_space?(place)
        @board[place].is_a? Integer
    end

    def put_marker(mark)
        @board[get_place] = mark
        show_board
    end

    def win?(player)
        ary = [[1, 2, 3], [4, 5, 6], [7, 8, 9], [1, 4, 7], [2, 5, 8], [3, 6, 9], [1, 5, 9], [3, 5, 7]]
        ary.any? do |array| 
            array.all? { |num| @board[num - 1] == player.mark }
        end
    end

    def draw_condition
        @board.all? { |num| num.is_a? String }
    end

    def end_game?
        if win?(@player_1)  #use other check. The comparison wit == true is not necessary.
            puts "Player 1 wins!"
            show_board  #Show end situation
            exit
        elsif win?(@player_2)
            puts "Player 2 wins!" 
            show_board  #Show end situation
            exit
        elsif draw_condition == true && player1_win_condition == false && player2_win_condition == false
            puts "It's a draw!"
            exit
        end
    end
end

class Player
    attr_accessor :mark

    def initialize(mark)
        @mark = mark
    end
end

new_board = Board.new(Player.new("X"), Player.new("O"))
new_board.play

I removed the reader place and the variable @place. There is no reason why you need it. It is only a temporary used variable, there is no need to store it in the class. I replaced it with local variables and method return parameters.

The use of the return command is not necessary. I used it here to show explicit when I want to return a value.

I integrated the checks into get_place. There is also a check for wrong values (like letters) and different messages.

I replaced player1_win_condition with a generic test with parameter.

I integrated the game in a method play. So it would be easier to start multiple games.

Most important: You used the fix constant 'X' and 'O' in your Game. But this data are also defined a parameter in the class Player. This can make problems if you replace X by x or something else.

What is missing: * There is no check for similar Player.marks

What could be checked/Modified:

  • I don't like the end_game? in the actual version. You could integrate it to play (player 1 can only win after his move, same for player 2. Then end_game? needs only to check for draw.
  • The board could be a simple array without initialization. nil would indicate free fields. This could make some checks easier. But with the initialization the board gets its initial values to be shown in show_board.

Another version!

The first version had some code repetition. For each player the code was written again.

The new version avoids this repetion:

class Board attr_reader :board

    def initialize(player1, player2)
        @board = (1..9).to_a    #You can use also a range to initialize the array
        @player_1 = player1
        @player_2 = player2
        puts "Player 1 is %s, Player 2 is %s." % [player1.mark, player2.mark]
        show_board
    end

    def play
      sequence = [@player_1,@player_2]
      until draw?
          puts "Player %s turn" % sequence.first.name
          put_marker(sequence.first.mark)
          if win?(sequence.first)
            puts "Player %s wins!" % sequence.first.name
            show_board  #Show end situation
            return 
          end
          sequence.rotate! #switch to next player
        end
        puts "It's a draw!" #end message if not left before, becaus eone player wins
    end

    def show_board
        puts '-'*9          #I prefer some lines in the board
        @board.each_slice(3) { |x| #Good! You use slice.
            puts x.join(' | ') 
            puts '-'*9
          }
    end

    def get_place
      while
          place = gets.to_i - 1 #Use a return parameter
        #Check also if you get only values between 1 and 9 (no letters, no 0)
          if ! place.between?(0,9)
              puts "Invalid move, use 1..9. Please choose again!"
              next
          end
          if ! free_space?(place) 
              puts "Invalid move, Pos. %i is occupied. Please choose again!"
              next
            end
            break #stop loop, Value is ok
        end
      return place
    end

    def free_space?(place)
        @board[place].is_a? Integer
    end

    def put_marker(mark)
        @board[get_place] = mark
        show_board
    end

    def win?(player)
      ary = [[1, 2, 3], [4, 5, 6], [7, 8, 9], [1, 4, 7], [2, 5, 8], [3, 6, 9], [1, 5, 9], [3, 5, 7]]
      ary.any? do |array| 
          array.all? { |num| @board[num - 1] == player.mark }
      end
    end

    def draw?
        @board.all? { |num| num.is_a? String }
    end

end

class Player
    attr_accessor :mark
    attr_accessor :name

    def initialize(name, mark)
        @name = name
        @mark = mark
    end
end

new_board = Board.new(Player.new('Fred', "X"), Player.new('Joe', "O"))
new_board.play

To identify the players I need also a name. Before you had the fix 'Player one'. You could also remove the text Player in the output (replace Player X turn with X turn).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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