Aisha's RPS Challenge #2128
Aisha's RPS Challenge #2128
Conversation
|
|
||
|
|
||
| feature "player has lost against the computer" do | ||
| scenario "When player plays paper" do |
There was a problem hiding this comment.
Tests look good and comprehensive, nice one!
| def initialize | ||
| @rules = { | ||
| Rock: { Rock: :draw, Paper: :lose, Scissors: :win }, | ||
| Paper: { Rock: :win, Paper: :draw, Scissors: :lose }, |
There was a problem hiding this comment.
I really like this way of going about determining the rules of the game
|
|
||
| get '/play' do | ||
| @username = session[:username] | ||
| @game = $game |
|
|
||
| $final_result = FinalResult.new | ||
| $final_result.calculate(@player_option, @computer_option) | ||
| @final_result = $final_result.winner |
There was a problem hiding this comment.
Could the @final_result variable go into a separate get route and have /game redirect to it (either as a post or get)? The global $final_result can stay in /game if needed?
leoht
left a comment
leoht
left a comment
There was a problem hiding this comment.
Really nice work - I left a few comments here and there to improve mainly the testing and stubbing side of things, and some of the code design in a few places. Let me know if you have any questions on the feedback!
| end | ||
|
|
||
| def winner | ||
| return "Congratulations! You won, you beat the computer!" if @result == :win |
There was a problem hiding this comment.
Rather than having human-readable messages returned by this class, it might be better to let this responsibility to the view (in the .erb file) to have all the content / text displayed to the user. This way, it becomes easier to change it, if we want to change the text displayed, translate it in other languages, etc. The method winner could then return either the result directly (:win, :lose, etc), and the the message to display would be part of the view layer
| session[:computer] = Computer.new(["Rock", "Paper", "Scissors"]) | ||
| @computer_option = session[:computer].option | ||
|
|
||
| $final_result = FinalResult.new |
There was a problem hiding this comment.
If the object FinalResult.new doesn't need to be accessed in a different route (which seems to be the case, as it's used only to calculate the result and chose the winer), it's probably good to avoid making it a global variable (it's generally best to avoid using global variables unless there is a need to):
final_result = FinalResult.new
final_result.calculate(@player_option, @computer_option)
@final_result = $final_result.winnerMaybe the name of the class could be changed to FinalResultCalculator as well, to avoid mixing up that object with the result value itself?
| @@ -0,0 +1,5 @@ | |||
| def sign_in_and_play | |||
| visit('/') | |||
There was a problem hiding this comment.
Nice use of a helper method here - a way to make it better could be to pass the player name as an argument, this way it becomes more explicit, when used in tests, what name is going to be used:
sign_in_and_play('Aisha')
click_button 'Rock'
expect(page).to have_content "Aisha played Rock"| <html> | ||
| <head> | ||
| </head> | ||
| <style> |
There was a problem hiding this comment.
It's usually best to indent the HTML code, so it's easier to read - this becomes really useful when dealing with larger and more complex HTML pages! You can use a VS code extension (or similar) to auto indent the code when editing HTML files
| @@ -0,0 +1,22 @@ | |||
| feature "display computer's option" do | |||
| scenario "when computer plays rock" do | |||
| allow_any_instance_of(Array).to receive(:sample).and_return 'Rock' | |||
There was a problem hiding this comment.
Rather than stubbing the return of Array.sample (which could be dangerous, as this would modify the behaviour of the Array's sample method for every single array used in your programs, even libraries, etc) - prefer stubbing a method of your own class you have more control of. For example, you could create a new method in Computer, called get_random_move, which would wrap the call to .sample - and then stub this method's return value
| context "and has won the game" do | ||
| it "it returns winning message" do | ||
| comp = Computer.new(["Rock", "Paper", "Scissors"]) | ||
| allow(comp).to receive(:option).and_return("Scissors") |
There was a problem hiding this comment.
Here, because you're passing directly the value of option to the method .calculate, it's not needed to stub the return value of comp.option, because you could directly give the plain string value to .calculate:
final_result.calculate(player.option, "Scissors")Stubbing would be useful in the case we'd inject the whole instance comp into that class, and then the method would be called by that class later on
3 participants
Completed the first group of user stories, need to refactor code and add to README.md