2
\$\begingroup\$

I have just completed my first Ruby OOP project. Fairly simple but still a bit time consuming as I am still learning the language.

I would like feedback and ways I can improve on this and make the flow of code better or could have done something in a different way to save time or the "right way".

I thank you all in advance!

my code below

class Player
    attr_accessor :name

    def initialize(name)
        @name = name
    end

end

class Game

    def player_vs_AI?
        puts "Hello User..."
        puts "Will you be playing against another person or the computer?"
        puts "Choose 1 for another person or choose 2 for the computer"
        @answer = gets.chomp.to_s

    end


    def greeting

        names = ['Bob', 'Joseph', 'Catherine', 'Paul', 'Tom', 'Baker']
        puts "Hi, please enter your name player 1: "
        name1 = gets.chomp
        @player1 = Player.new(name1)

        if @answer == '1'
            puts "Hi, please enter your name player 2: "
            name2 = gets.chomp
            @player2 = Player.new(name2)
        elsif @answer == '2'
            name2 = names.sample
            @player2 = Player.new(name2)
            puts "Player 2 will be named #{@player2.name}"
        end
        sleep(0.5)
        puts "Welcome to Rock-Paper-Scissor"
        sleep(0.5)
        puts "Welcome #{@player1.name} and #{@player2.name}\n\n"
        sleep(0.5)

    end 

    def instruct
        puts "The rules of the game are simple\n"
        puts "Simply choose Rock(R), Paper(P) or Scissor(S) each turn\n "
        puts "Paper beats Rock, Scissor beats paper and Rock beats Scissor\n"
        puts "Ready....?"
        sleep(1.0)
    end

    def choice
        choices = ['r', 'p', 's']

        puts "#{@player1.name}, please choose your choice of weapon: R(rock), P(paper), S(Scissor)"
        @choice_1=gets.chomp
        @choice_1.downcase!

        while !(@choice_1 == 'r' || @choice_1 == 'p' || @choice_1 == 's') do
            puts "#{@player1.name} That is not a valid choice, please choose from r, p, or s"
            @choice_1=gets.chomp
            @choice_1.downcase!
        end

        if @answer == '1'
            puts "#{@player2.name}, please choose your choice of weapon: R(rock), P(paper), S(Scissor)"
            @choice_2=gets.chomp
            @choice_2.downcase!

            while !(@choice_2 == 'r' || @choice_2 == 'p' || @choice_2 == 's') do
                puts "#{@player2.name} That is not a valid choice, please choose from r, p, or s"
                @choice_2=gets.chomp
                @choice_2.downcase!
            end
        elsif @answer == '2'
            @choice_2 = choices.sample
        end         
    end

    def logic
        puts "Lets see who wins...will it be #{@player1.name} or will it be #{@player2.name}\n"
        sleep(2.0)
        if @choice_1 == @choice_2
            puts "Opps, both players picked the same weapon, It is a TIE!!!"
            puts "Lets go back and choose again..."
            sleep(3.0)
            choice
        end
        if @choice_1 == 'r' && @choice_2 == 'p'
            puts "#{@player2.name} win!!! Paper covers Rock"
        elsif @choice_1 == 'p' && @choice_2 == 'r'
            puts "#{@player1.name} you Win!!! Paper covers Rock"
        elsif @choice_1 == 'p' && @choice_2 == 's'
            puts "#{@player2.name} you win!!! Scissor cuts Paper "
        elsif @choice_2 == 's' && @choice_1 == 'p'
            puts "#{@player1.name} you win!!! Scissor cuts Paper"
        elsif @choice_1  == 'r' && @choice_2 == 's'
            puts "#{@player1.name} you win!!! Rock beats Scissor"
        else @choice_2 == 'r' && @choice_1 == 's'
            puts "#{@player2.name} you win!!!! Rock beats Scissor"
        end
    end

    def play_again?
        sleep(0.5)
        puts "\n"
        puts "Would you like to play again?(yes/no)  "
        input = gets.chomp
        if input == 'yes'
            start
        elsif input == 'no'
            puts "Thank #{@player1.name} and #{@player2.name} for playing!"
        end
    end

end

def start
    game = Game.new
    game.player_vs_AI?
    game.greeting
    game.instruct
    game.choice
    game.logic
    game.play_again?
end 
start
```
\$\endgroup\$
  • \$\begingroup\$ You can simplify the task of determining which player, if either, wins a given throw, by creating a hash winners = { r: :s, :p :r, :s :p }. Then if player 1 throws p1 (:r, :p or :s) and player 2 throws p2, player 1 wins if winners[p1] == p2, else player 2 wins if winners[p2] == p1 else it's a tie and they throw again. \$\endgroup\$ – Cary Swoveland Feb 27 at 3:49
3
\$\begingroup\$

The biggest thing that will allow the code to clean up a fair bit is if you had made a subclass of Player for AIPlayer:

class AIPlayer < Player
  NAMES = ['Bob', 'Joseph', 'Catherine', 'Paul', 'Tom', 'Baker']

  def initialize
    super NAMES.sample
  end
end

that will allow you to add a method get_weapon to Player, which would ask for the user input and override it in AIPlayer to return a random selection. Your choice method then can become cleaner:

def choice
  @choice_1 = @player1.get_weapon
  @choice_2 = @player2.get_weapon
end

and not care about if it's player vs player or vs ai. Your player_vs_AI? method can then return true or false and not have to assign an instance variable at all:

def player_vs_AI?
  puts "Will you be playing against another person or the computer?"
  puts "Choose 1 for another person or choose 2 for the computer"

  gets.chomp.to_s == '2'
end

and can be then used to clean up like:

def greeting
  puts "Hi, please enter your name player 1: "
  name1 = gets.chomp
  @player1 = Player.new(name1)

  if player_vs_AI?
    @player2 = AIPlayer.new
    puts "Player 2 will be named #{@player2.name}"
  else
    puts "Hi, please enter your name player 2: "
    name2 = gets.chomp
    @player2 = Player.new(name2)
  end
  sleep(0.5)
  puts "Welcome to Rock-Paper-Scissor"
  sleep(0.5)
  puts "Welcome #{@player1} and #{@player2}\n\n"
  sleep(0.5)
end

and you no longer need to call player_vs_AI? during start.

Your play_again? method also shouldn't call start itself, instead the start method should check if they want to play again and just keep getting choices and checking the results:

def start
  game = Game.new
  game.greeting
  game.instruct
  loop do
    game.choice
    game.logic
    break unless game.play_again?
  end
end

having trimmed down the choice method, it could be combined with the logic method and perhaps renamed play (or something more meaningful) and start could be renamed run.

On a last note, I would try to make things consistent for the user. You're asking for input options 3 times, the first it's enter 1 or 2 (player_vs_AI?), the second it's r, p, or s (choice) and the last time in play_again? they have to enter yes or no. Only in choice is there input validation (what happens in play_again? if I try to enter y instead of yes?). This could be more consistent (always allow the full word to be entered, or just the first letter, for instance).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ awesome! thank you for the feedback and great suggestions that i will make in my code. Also, do you think there would have been a better way to do my logic method instead of all the if-else loops \$\endgroup\$ – nimpa Jan 2 at 14:11
2
\$\begingroup\$

I am not a professional game developer, just trying to help you base on my experience with Ruby so far. Glad to see your willingness of coding correctly from the beginning.

Some points to improve:

1.Move the bot names and game mode options to CONSTANTS

2.Prompting for user's input is duplicated many times, you should make a function that take prompting lines and accepted values as arguments then return only when user input is in accepted values.

3.Beside name, a Player can have weapon as its instance variable also.

4.About the logic method, I found a way to improve it, please consider. If we put all the options to an array by a specific order that the option at higher index will win ([r, p, s] or [p, s, r] or [s, r, p]) then the logic can patternized. The only exception is when the higher index is the last element but the smaller index is the first element, we can solve the exception with a simple if

5.You should also learn how to write test spec and apply it here especially for logic method

6.Last but not least, your indentation :)

class Player
  attr_accessor :name, :weapon

  def initialize(name)
    @name = name
  end
end

class Game
  BOT_NAMES = ['Bob', 'Joseph', 'Catherine', 'Paul', 'Tom', 'Baker']
  WEAPONS   = {'r' => 'Rock', 'p' => 'Paper', 's' => 'Sissors'}

  def prompt_for_user_input(accepted_inputs=[], prompt_lines=[], is_case_sensitive=true)
    loop do
      prompt_lines.each { |line| puts line }
      user_input = gets.chomp
      return user_input.to_s if accepted_inputs.empty?
      is_valid_input = is_case_sensitive ? accepted_inputs.include?(user_input) : accepted_inputs.any?{ |s| s.casecmp(user_input) == 0 }
      return user_input.to_s if is_valid_input
    end
  end

  def mode_selection
    prompt_lines = [
      "Hello User...",
      "Will you be playing against another person or the computer?",
      "Choose 1 for another person or choose 2 for the computer"
    ]

    @mode = prompt_for_user_input(['1', '2'], prompt_lines)
  end

  def player_creation
    @player1 = Player.new(prompt_for_user_input([], ["Hi, please enter your name player 1:"]))
    @player2 = if (@mode == '1')
      Player.new(prompt_for_user_input([], ["Hi, please enter your name player 2:"]))
    else
      bot_name = BOT_NAMES.sample
      puts "Player 2 will be named #{bot_name}"
      Player.new(bot_name)
    end

    sleep(0.5)
    puts "Welcome #{@player1.name} and #{@player2.name} to Rock-Paper-Scissor"
  end

  def instruct
    puts "The rules of the game are simple\n"
    puts "Simply choose Rock(R), Paper(P) or Scissor(S) each turn\n "
    puts "Paper beats Rock, Scissor beats Paper and Rock beats Scissor\n"
    puts "Ready....?"
    sleep(1.0)
  end

  def weapon_selection
    @player1.weapon = prompt_for_user_input(WEAPONS.keys, ["#{@player1.name}, please chooose your choice of weapon: r(Rock), p(Paper), s(Sissor)"], false)
    @player2.weapon = if (@mode == '1')
      prompt_for_user_input(WEAPONS.keys, ["#{@player2.name}, please chooose your choice of weapon: r(Rock), p(Paper), s(Sissor)"], false)
    else
      bot_weapon = WEAPONS.keys.sample
      puts "#{@player2.name} chooses #{WEAPONS[bot_weapon]}"
      bot_weapon
    end

    puts "Let see who wins... will it be #{@player1.name} or will it be #{@player2.name}"
    sleep(2.0)
    decider
  end

  def decider
    if @player1.weapon == @player2.weapon
      puts "Opps, both players picked the same weapon, It is a TIE!!!"
      puts "Lets go back and choose again..."
      sleep(3.0)
      weapon_selection
    end

    weapon_keys = WEAPONS.keys
    weapon_1_index = weapon_keys.find_index(@player1.weapon)
    weapon_2_index = weapon_keys.find_index(@player2.weapon)
    if weapon_1_index > weapon_2_index
      if (weapon_1_index == weapon_keys.length - 1) && (weapon_2_index == 0)
        @winner, @loser = @player2, @player1
      else
        @winner, @loser = @player1, @player2
      end
    else
      if (weapon_2_index == weapon_keys.length - 1) && (weapon_1_index == 0)
        @winner, @loser = @player1, @player2
      else
        @winner, @loser = @player2, @player1
      end
    end

  end

  def result 
    puts "#{@winner.name} you win !!! #{WEAPONS[@winner.weapon]} beats #{WEAPONS[@loser.weapon]}"
  end

  def again?
    prompt_for_user_input(['y', 'n'], prompt_lines=["Would you like to play again?(y/n)"]) == 'y'
  end

  def play
    loop do
      mode_selection
      player_creation
      instruct
      weapon_selection
      decider
      result
      break unless again?
    end
    puts "Thank #{@player1.name} and #{@player2.name} for playing!"
  end
end

g = Game.new
g.play
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Awesome! All the suggestions were great and made my code a lot better. I do have a question for the logic method that you suggested. When you wrote weapon_keys.length - 1 is that because we are comparing it to the index of the array which starts at 0 so to make it equal we have to do .length-1? \$\endgroup\$ – nimpa Jan 2 at 17:51
  • \$\begingroup\$ No, the weapon_keys.length - 1 is to check if the weapon is the last element of the array. Last index of the last element is array length - 1, a common trick when working with array \$\endgroup\$ – Thang Jan 3 at 4:00
  • \$\begingroup\$ Update: All the Game methods except for play should be private \$\endgroup\$ – Thang Jan 3 at 7:31

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.