Skip to content

Farzan's RPS game (2 user stories complete)#2109

Open
Farzan-I wants to merge 12 commits into
makersacademy:mainfrom
Farzan-I:main
Open

Farzan's RPS game (2 user stories complete)#2109
Farzan-I wants to merge 12 commits into
makersacademy:mainfrom
Farzan-I:main

Conversation

@Farzan-I
Copy link
Copy Markdown

@Farzan-I Farzan-I commented May 8, 2022

Farzan Imanzadeh

Completed the below user stories:

As a marketeer
So that I can see my name in lights
I would like to register my name before playing an online game

As a marketeer
So that I can enjoy myself away from the daily grind
I would like to be able to play rock/paper/scissors

Did not complete the bonus parts (multiplayer and RPSLS)

Comment thread lib/game.rb
@win = nil
@message = ""
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this feels like neat logic - very easy to read

Comment thread lib/game.rb
end

private

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could consider keeping the computer in a separate class


expect(page).to have_content("Farzan Vs. Computer")
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good, specific content requirements throughout the tests - I need to tighten mine up.

Copy link
Copy Markdown

@eoinmakers eoinmakers left a comment

Choose a reason for hiding this comment

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

Nice work on this! You've demonstrated the learning objectives of the week well.

My feedback revolves around refactoring to remove duplication of code - let me know if you have any questions.

You have a good range of unit & feature tests here, inc. stubbing the computer's move. Nice work! Fully comprehensive unit testing for this challenge would include unit tests for every combination of player/computer moves.

Comment thread app.rb
@game = $game
@game.scissors
session[:message] = @game.message
redirect '/play'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Currently, you define a route for each player move option, and each of these routes uses mostly the same code. This is a duplication of code that could be refactored.

Another method of approach this logic is to pass in the player move as a parameter, calling the same route, and passing that param into the @GAMe instance.

Comment thread lib/game.rb
when nil
@message = "It's a tie! Try again!"
end
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This approach can be simplified to make this class more concise. Check out the following approach: https://github.com/makersacademy/rps-challenge/blob/main/docs/review.md#use-of-ifelsif-conditionals-for-business-logic

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