-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sophie gilder RPS #2136
base: main
Are you sure you want to change the base?
Sophie gilder RPS #2136
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, well done! Sorry for the delay in reviewing this - not much to add aside a few small suggestions here and there. Feel free to ask me if you have more questions on that feedback :)
|
||
get "/play" do | ||
@game = $game | ||
erb @game.select_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually it's best to enforce the view name to be something specific, rather than the return of a method that you might not fully control — for example, tomorrow you (or someone else) might modify select_type
to return a different value, but might also forget to add a new view to cover this case here, which would break the program. Rather than coupling tightly the return value to the view name, it might be better to add some condition and specifically select the correct view — or better, to have a generic single view that is always used, but using an instance variable and some dynamic ERB tags to display a slightly different portion of the HTML, depending on the value
|
||
get "/result" do | ||
@game = $game | ||
erb @game.result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark than above here — you could set the result in an instance variable and display something different in the view depending on this
|
||
### Routes | ||
|
||
````ruby |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good you've designed the routes beforehand here — it probably helped a lot your decisions in terms of routing, and how the application would behave, HTTP-wise
|
||
def initialize | ||
@weapon = Game::WEAPONS.sample | ||
@name = :Computer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor one - usually symbol names are always lowercase
@@ -0,0 +1,19 @@ | |||
def enter_and_submit_name_single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of helper methods here — to improve it, something good could be to add arguments (e.g to pass the player name) so, when calling the helper in your tests, the names the tests should expect become more explicit:
# Example
enter_and_submit_name_single('Rosie')
click_button "Scissors"
expect(page).to have_text "Rosie selected Scissors"
# ...
expect(Game::RULES).to eq ({rock: :scissors, scissors: :paper, paper: :rock}) | ||
end | ||
|
||
it "returns true for single player" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to have a test as well that checks it returns false
when it is not a single player
No description provided.