rps challenge #1036
rps challenge #1036
Conversation
There was a problem hiding this comment.
Hi Marianne.
I've taken a look at your submission for the RPS challenge and left a few comments below.
The main question you raised in your self review was around your use of two 'paths' through your game for single and multi player options.
While the implementation you've used here definitely works and gets the job done, you're right to recognise that it might not be the most efficient route. Imagine a situation is which you wanted to extend your game to be playable by three players - you'd have to duplicate all of your effort again to make an entirely new path through the app.
Ideally, you want your application to run the same, regardless of the number of human players (bear in mind that a computer player is, ultimately just another player which has it's choice automated).
A good first step would be to remove your Computer class, and make it so that the Player class can operate the same whether it's controlled by a human or by the computer.
Once you've done this, you'll need, at some point, for your game to be able to check whether player two is a human, and decide whether or not to give them the option to make a selection, or just assign one at random.
If you'd like to talk more about how you might make this work then let me know!
| - Having finished implementing its functionality ,I used css to make it look nice | ||
|
|
||
|
|
||
| [Play now!](https://stark-journey-20582.herokuapp.com/) |
lib/computer.rb
Outdated
| @@ -0,0 +1,12 @@ | |||
| class Computer | |||
There was a problem hiding this comment.
Computer feels like a potentially unnecessary class here. Ideally, the application shouldn't care if a player in the game is a human or a computer - the Player class could allow for an option to be picked, or simply generate one at random if not option is chosen.
spec/features/homepage_spec.rb
Outdated
| expect(page).to have_content 'ROCK PAPER SCISSORS LIZARD SPOCK!' | ||
| end | ||
|
|
||
| scenario 'user can choose 1 player' do |
There was a problem hiding this comment.
This test and the one after it feel a little redundant. If the button doesn't exist, then other feature tests which rely on the user being able to click the button will fail, so you test the existence of the button implicitly.
I think these types of tests are a great stepping stone to use as part of the TDD process, but they can - and should - be deleted once you have better quality tests in place.
spec/unit/computer_spec.rb
Outdated
| end | ||
| end | ||
|
|
||
| context '#choice' do |
There was a problem hiding this comment.
These tests aren't really doing anything.
You're explicitly telling your array to return a specific value, and then checking if that value has been returned. 🙂
spec/unit/game_spec.rb
Outdated
| end | ||
|
|
||
| context '#play versus computer' do | ||
| context 'computer choice is paper' do |
There was a problem hiding this comment.
While this is definitely super well tested, I don't think you need to test every possible outcome of the game - if you later wanted to add further options this could quickly get to be unmanageable 🙂
I think a test for a draw, a test for a player one win, and a test for a player two win would be enough.
spec/unit/game_spec.rb
Outdated
| end | ||
| end | ||
|
|
||
|
|
There was a problem hiding this comment.
Layout/EmptyLines: Extra blank line detected.
No description provided.