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

Aisha's RPS Challenge #2128

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

Conversation

Aisha-Yusuff
Copy link

Completed the first group of user stories, need to refactor code and add to README.md



feature "player has lost against the computer" do
scenario "When player plays paper" do

Choose a reason for hiding this comment

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

Tests look good and comprehensive, nice one!

def initialize
@rules = {
Rock: { Rock: :draw, Paper: :lose, Scissors: :win },
Paper: { Rock: :win, Paper: :draw, Scissors: :lose },

Choose a reason for hiding this comment

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

I really like this way of going about determining the rules of the game


get '/play' do
@username = session[:username]
@game = $game

Choose a reason for hiding this comment

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

Is $game variable used or needed?


$final_result = FinalResult.new
$final_result.calculate(@player_option, @computer_option)
@final_result = $final_result.winner

Choose a reason for hiding this comment

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

Could the @final_result variable go into a separate get route and have /game redirect to it (either as a post or get)? The global $final_result can stay in /game if needed?

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.

Really nice work - I left a few comments here and there to improve mainly the testing and stubbing side of things, and some of the code design in a few places. Let me know if you have any questions on the feedback!

end

def winner
return "Congratulations! You won, you beat the computer!" if @result == :win
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than having human-readable messages returned by this class, it might be better to let this responsibility to the view (in the .erb file) to have all the content / text displayed to the user. This way, it becomes easier to change it, if we want to change the text displayed, translate it in other languages, etc. The method winner could then return either the result directly (:win, :lose, etc), and the the message to display would be part of the view layer

session[:computer] = Computer.new(["Rock", "Paper", "Scissors"])
@computer_option = session[:computer].option

$final_result = FinalResult.new
Copy link
Contributor

Choose a reason for hiding this comment

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

If the object FinalResult.new doesn't need to be accessed in a different route (which seems to be the case, as it's used only to calculate the result and chose the winer), it's probably good to avoid making it a global variable (it's generally best to avoid using global variables unless there is a need to):

final_result = FinalResult.new
final_result.calculate(@player_option, @computer_option)
@final_result = $final_result.winner

Maybe the name of the class could be changed to FinalResultCalculator as well, to avoid mixing up that object with the result value itself?

@@ -0,0 +1,5 @@
def sign_in_and_play
visit('/')
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 a helper method here - a way to make it better could be to pass the player name as an argument, this way it becomes more explicit, when used in tests, what name is going to be used:

sign_in_and_play('Aisha')
click_button 'Rock'
expect(page).to have_content "Aisha played Rock"

<html>
<head>
</head>
<style>
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 usually best to indent the HTML code, so it's easier to read - this becomes really useful when dealing with larger and more complex HTML pages! You can use a VS code extension (or similar) to auto indent the code when editing HTML files

@@ -0,0 +1,22 @@
feature "display computer's option" do
scenario "when computer plays rock" do
allow_any_instance_of(Array).to receive(:sample).and_return 'Rock'
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than stubbing the return of Array.sample (which could be dangerous, as this would modify the behaviour of the Array's sample method for every single array used in your programs, even libraries, etc) - prefer stubbing a method of your own class you have more control of. For example, you could create a new method in Computer, called get_random_move, which would wrap the call to .sample - and then stub this method's return value

context "and has won the game" do
it "it returns winning message" do
comp = Computer.new(["Rock", "Paper", "Scissors"])
allow(comp).to receive(:option).and_return("Scissors")
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, because you're passing directly the value of option to the method .calculate, it's not needed to stub the return value of comp.option, because you could directly give the plain string value to .calculate:

final_result.calculate(player.option, "Scissors")

Stubbing would be useful in the case we'd inject the whole instance comp into that class, and then the method would be called by that class later on

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