Skip to content
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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Conversation

sophiechannon
Copy link

No description provided.

Copy link
Contributor

@leoht leoht left a 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants