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

Stevie Spiegl RPS #2115

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

Stevie Spiegl RPS #2115

wants to merge 62 commits into from

Conversation

S-Spiegl
Copy link

@S-Spiegl S-Spiegl commented May 9, 2022

No description provided.

views/play.erb Outdated
@@ -23,7 +23,6 @@ border='2' name='player_choice' type='submit' id='paper'></>
<input type="hidden" name="player_choice" value="paper" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to indent everything! Makes it easier to read for others ;) Maybe even have blank lines between the parts that don't relate i.e. After each form to show where one stops and the other starts (this is only when you know a code review will be done)

views/form.erb Outdated
@@ -9,7 +9,7 @@ h1 {text-align: center; font-family: arial; padding-top: 20px}
</style>

<h1>Enter Name</h1>
<form action='/name' method='post'>
<form action='/play' method='post'>
<input type="text" name="player" style="padding: 5px 15px"><br><br>
<button type="submit" style="padding: 5px 10px">Submit name!</button>
</form>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation.. naughty naughty!

@@ -16,7 +16,7 @@ text-align: center;}
<h1>You chose <%= @player_weapon %>...<br></h1>
<h1>Computer chose <%= @computer_weapon %>...<br><h1>
<h1><%= @message %><h1>
<form action='/name_play_again' method='post'>
<form action='/play_again' method='post'>
<button type="submit" style="padding: 5px 10px">Have another go?</button>
</form>
</body>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same again. Takes maybe a day or two to write the code, but people would have to read it an unspecified amount of time

@@ -5,7 +5,4 @@
it "should return player's name" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much better indentation here. Clear class names and tests. Nice one man!

xit 'displays a congratulatory message if the player wins' do
end
xit 'taunts the player if they lose' do
end
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty file? I think you left the spec for this somewhere else? Then maybe you could delete this file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it would be good to consider what you might have tested in here if you had the time - you could even comment some ideas and come back to it at a later stage when you want to make improvements, especially the logic in Game is an important one.

click_on(id='rock')
expect(page).to have_content "You did not choose wisely."
end

scenario 'scissors beats paper' do
allow_any_instance_of(Computer).to receive(:weapon).and_return(:paper)
sign_in_and_play
sign_in
click_on(id='scissors')
expect(page).to have_content "You chose wisely."
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really clear test scenarios, really enjoyed reading these if I'm honest. Good takeaway for my tests in the future, thank you!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, good testing, well done for using stubs effectively here

expect(current_path).to eq('/play')
end

scenario 'player can see their name on the play page' do
sign_in_and_play
sign_in
expect(current_path).to eq('/play')
expect(page).to have_content('Alien')
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear, concise, good indentation

# run the model in the controller. Take args such as computer (contained in model)
# player choice (a param), and use them with the if/else statement in the engine
# to return a winner which can be interpolated in the view

# Start the server if this file is executed directly (don't change the line below)
run! if app_file == $0
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great file, concise, good use of class names and instance and global variables

Copy link

@z-rasool z-rasool left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good file structure, use of stubs and identification of which objects to create. It would have been good to see some unit tests for Game, and perhaps something you can come to if you get the time in the future! Well done Stevie.

elsif @player_weapon == :paper && @computer_weapon == :scissors
"You did not choose wisely."
end
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see the creation of a WEAPONS constant in this file. Could the engine method be refactored to reduce repetition - perhaps the use of a hash?

click_on(id='rock')
expect(page).to have_content "You did not choose wisely."
end

scenario 'scissors beats paper' do
allow_any_instance_of(Computer).to receive(:weapon).and_return(:paper)
sign_in_and_play
sign_in
click_on(id='scissors')
expect(page).to have_content "You chose wisely."
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, good testing, well done for using stubs effectively here

xit 'displays a congratulatory message if the player wins' do
end
xit 'taunts the player if they lose' do
end
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it would be good to consider what you might have tested in here if you had the time - you could even comment some ideas and come back to it at a later stage when you want to make improvements, especially the logic in Game is an important one.

def select_weapon(weapon)
@weapon = weapon
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the WEAPONS constant be used in this file too?

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.

3 participants