halfghaninne/actual-rock-paper-scissors

Rock Paper Scissors assignment 2015-01-29

actual-rock-paper-scissors's Issues

5 Reflections for Alex

3 feedback items and 2 descriptions with a little feedback!

1. Feedback:
   It looks like you've got several 'require_relative' lines in separate files that are the same. For example, line 2 of both 'rock-paper-scissors.rb' and 'gamedriver.rb' is:

require_relative 'player'

Since 'gamedriver.rb' also has "require_relative 'rock-paper-scissors.rb' ", you don't need to include "require_relative 'player.rb' " in 'rock-paper-scissors.rb.' It just has to be required once.

1. Feedback:
   When setting the number of rounds, you should call '.to_i' on it right away, instead of sending it to RockPaperScissors and then converting it on line 39 (in rock-paper-scissors.rb). What if you had to send this number other places? Would you want to convert every time it was received?
2. Feedback:
   You appear to be creating your player objects correctly (line 36-36, rock-paper-scissors.rb). What about line 37 in 'gamedriver.rb'? Also, you've got a lot going on in your initialize method in the RockPaperScissors class. You should definitely consider how you could split this up into different methods.
3. Description:
   So it looks like you plan on running games by creating Game objects. When initialized the Game class creates player objects, and has them play a game. It looks like the player moves are stored inside of the game object. If one player wins, their score is increased with add_point. Could this be set up so the player also stores their moves, in addition to their score? This could be useful if you wanted to do other things with their moves, like verify them in a different class.
4. Description:
   You're logic to check the winner is included in the Game initialize method. You should definitely check out the Ruby Exercise in the Prep Work that asked us to implement this check using hashes. It shrinks down the code and improves the readability significantly. This is especially useful if you plan on making Rock,Paper,Scissors,Lizard,Spock.

Overall, it looks like you're on the right track!

PS. I've numbered it 1-5, but for some reason, Github is changing the numbering to 1,1,2,3,4

gamedriver.rb feedback

# puts "Here are the rules of the game:\n\n#{Game.rules}\n\n" could work if Game.rules was changed to rules. Since initialize and rules are methods in the same object class you don't need to call rules on a Game object, just call the method.
I'm not 100% certain this is correct, but it's worth trying out.

rockpaperscissors.rb feedback

if n % 2 == 0 winner = (n / 2) + 1 elsif n % 2 != 0 winner = (n / 2).ceil end(lines 45-49) can be refactored to be simpler if you only use integers. On line 35 of gamedriver if you have @number_of_rounds = gets.to_i and pass that to game, any float the user enters is auto rounded down when converted to an integer, and the code winner = (n / 2) + 1 from line 46 will work on both even numbers and odd numbers. Example, 3 / 2 = 1(1.5 rounded down to integer) +1 = 2 games needed to win. 4 / 2 = 2 + 1 = 3 games needed to win. Your way works perfectly, but this is a bit simpler and doesn't have to deal with floats.

Feedback For actual-rock-paper-scissors/rock-paper-scissors.rb

a) rockpaperscissors.rb
On line 85 of rock-paper-scissors, you ask the advise the user to re-enter the move. This is a nice feature, I just think it could be refined:

When the loop is run, it requires the player who entered an invalid move, and the player who entered valid move to re-enter their move. I think it would be more DRY if only the user who entered the invalid move be required to re-enter their move. I think this could be accomplished by testing the moves prior to running the loop.
Maybe you could write a until loop above the “if move1==move2” For example:

starting at line 57 to test player 1:

until valid_moves.inclue?(move1)
puts “That is not a valid move, please re-enter your move:”
puts “ "
move1 = gets.chomp.downcase
puts “ "
end

until valid_moves.inclue?(move2)
puts “That is not a valid move, please re-enter your move”
move1 = gets.chomp.downcase
end

if move1 == move2
puts “Tie!”

continue rest of code!

b) rockpaperscissors.rb
A rather light improvement to the readability of your program is to change the negative symbol “-“ to an equals sign, followed by the number of wins, followed by the word “win(s)”. The line I’m referring to is line 89 of rock-paper-scissors.rb. The way it currently reads could be confusing to someone unfamiliar with your program. I initially thought the program might have been subtracting something. After reading it twice I understood it fine though. Example with the improvement:

So far: Jimmy = 2 win(s), Tommy = 1 win(s)

c)rockpaperscissors.rb
I love the feature that catches whether a float or an integer is entered. It always rounds down to the nearest whole number. Super sweet!
However, I think the do two additional things:
If a player enters 2.3 rounds, it should display 2 rounds. The way it’s currently written, it will display 2.3, which isn’t quite accurate.
Your math then says that 2.3 rounds means the players score must be 2. That’s not accurate. A player could technically win with only 1. Here’s why: a.) if there are two rounds and two draws, or b.) if there are two rounds, one draw, and one win.
Still, it’s a very nice feature and you should keep it and just modify it.

d)rockpaperscissors.rb
I ran the code to test for a scenario where there are two rounds and no wins. I really think this should go into a tie breaker mode. The game declared by the player is technically over, but the program isn’t making the player aware of it. The tiebreaker would be an easy fix since your code is so concise. Simply add a tiebreaker prompt after the number of desired rounds has been reached, and continue your code a normal. You could simply add a counter to the bottom of the existing code and run an if test starting on line 88 after your other tests are done:

starting line 50:

counter = 0

stop next line

start up again line 88:

counter += 1
if counter > winner # i.e. the number of rounds
puts “TIE BREAKER!”
end

stop on the next line

So, until the counter is greater than the number of rounds to be played, it won’t print the Tie Breaker statement. I might change mine to work like this as well…

e) rockpaperscissors.rb
I think the rest of your program is very DRY and concise. One more comment though:

I feel you should break yor code into one or two more methods. One could be for lines 45 - 49. Turn that into a method that could be used on any program you run. Maybe put it in a separate file. Call it something like “validate_rounds.rb” or something like that. Then just call that method any time you need to validate the number of rounds a player seeks to play.