Recently I started programming with python. Today I was trying to make a stone, paper, scissor game. After so scratching my head long time, finally I got a working code.
from random import choice
def play_again():
print "Do you want to play again:- Choose 'yay' or 'nay'"
user_again = raw_input('->')
if user_again == 'yay':
SPCa()
elif user_again == 'nay':
print "Ok bye! hope you enjoyed the game. See you soon! :)"
else:
print "Please choose correct option."
play_again()
def SPCa():
computer_choice = choice( ['stone', 'paper', 'scissor'] )
computer_choosed = "Computer choosed %s" % computer_choice
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = raw_input('->')
if user_choice == computer_choice:
print computer_choosed
print "So it's a tie"
play_again()
elif user_choice == 'stone':
if computer_choice == 'paper':
print computer_choosed
print "So, You loose"
play_again()
elif computer_choice == 'scissor':
print computer_choosed
print "So, Cheers! You won!"
play_again()
elif user_choice == 'paper':
if computer_choice == 'scissor':
print computer_choosed
print "So, you loose."
play_again()
elif computer_choice == 'stone':
print computer_choosed
print "So, Cheers! You won!"
play_again()
elif user_choice == 'scissor':
if computer_choice == 'stone':
print computer_choosed
print "So, you loose."
play_again()
elif computer_choice == 'paper':
print computer_choosed
print "So, Cheers! You won!"
play_again()
else:
print "please choose correct option"
SPCa()
def SPC():
computer_choice = choice( ['stone', 'paper', 'scissor'] )
computer_choosed = "Computer choosed %s" % computer_choice
print "You are playing Stone, Paper, Scissor."
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = raw_input('->')
if user_choice == computer_choice:
print computer_choosed
print "So it's a tie"
play_again()
elif user_choice == 'stone':
if computer_choice == 'paper':
print computer_choosed
print "So, You loose"
play_again()
elif computer_choice == 'scissor':
print computer_choosed
print "So, Cheers! You won!"
play_again()
elif user_choice == 'paper':
if computer_choice == 'scissor':
print computer_choosed
print "So, you loose."
play_again()
elif computer_choice == 'stone':
print computer_choosed
print "So, Cheers! You won!"
play_again()
elif user_choice == 'scissor':
if computer_choice == 'stone':
print computer_choosed
print "So, you loose."
play_again()
elif computer_choice == 'paper':
print computer_choosed
print "So, Cheers! You won!"
play_again()
else:
print "please choose correct option"
SPCa()
SPC()
But I found my code so repetitive, it has not even a single loop. Please suggest how can I improve my code. What are things that I should learn?
3 Answers 3
Little issues
Your indentations seems wrong : too many spaces in a few places, not enough in a few other places. Whitespaces matter in Python so this is definitly something you should fix before going any further.
Style
Python has a style guide called PEP 8 which is definitly worth reading and and worth following if you do not have good reasons not to. In you case, your usage of whitespaces around parenthesis and the trailing whitespaces for instance are not compliant to PEP8. You'll find tools online to check your code compliancy to PEP8 in a automated way if you want to. This could also help you to detect and fix your indentation issues.
Don't Repeat Yourself
You've already realised that your code was repeating itself and that is was a bad thing. Let's see how this can be improved.
The only difference between SPCa
and SPC
is that SPC
has an additional line printed at the beginning. It might be easier to replace all the calls to SPC()
by a call to print
and then a call to SPCa
. Once this is done, we can get rid of SPC
and maybe rename SPCa
in play_game
.
At this stage, the code looks like:
from random import choice
def play_again():
print "Do you want to play again:- Choose 'yay' or 'nay'"
user_again = raw_input('->')
if user_again == 'yay':
play_game()
elif user_again == 'nay':
print "Ok bye! hope you enjoyed the game. See you soon! :)"
else:
print "Please choose correct option."
play_again()
def play_game():
computer_choice = choice(['stone', 'paper', 'scissor'])
computer_choosed = "Computer choosed %s" % computer_choice
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = raw_input('->')
if user_choice == computer_choice:
print computer_choosed
print "So it's a tie"
play_again()
elif user_choice == 'stone':
if computer_choice == 'paper':
print computer_choosed
print "So, You loose"
play_again()
elif computer_choice == 'scissor':
print computer_choosed
print "So, Cheers! You won!"
play_again()
elif user_choice == 'paper':
if computer_choice == 'scissor':
print computer_choosed
print "So, you loose."
play_again()
elif computer_choice == 'stone':
print computer_choosed
print "So, Cheers! You won!"
play_again()
elif user_choice == 'scissor':
if computer_choice == 'stone':
print computer_choosed
print "So, you loose."
play_again()
elif computer_choice == 'paper':
print computer_choosed
print "So, Cheers! You won!"
play_again()
else:
print "please choose correct option"
play_game()
print "You are playing Stone, Paper, Scissor."
play_game()
if main guard
In Python, it is a good habit to move your code actually doing things (by opposition to merely defining things) behind an if __name__ == "__main__":
guard. This is useful if you want to reuse the code : you can import the file and get all the benefits from it (the definition of values/functions/classes) without having it performing unwanted actions.
In your code, the end of the script becomes :
if __name__ == "__main__":
# execute only if run as a script
print "You are playing Stone, Paper, Scissor."
play_game()
Now we can get into the actual changes in your code. One of the issue it that you have multiple functions calling each other which make things difficult to understand.
All branches in play_game
end up calling play_again
(except when the option is not correct). It may be easier to call it once, at the end of the function like this.
else:
print "please choose correct option"
play_game()
return
play_again()
However, an even more simple option would be to check at the beginnign that the value is correct. You could define a list with the correct options and use it like this :
from random import choice
game_options = ['stone', 'paper', 'scissor']
def play_again():
print "Do you want to play again:- Choose 'yay' or 'nay'"
user_again = raw_input('->')
if user_again == 'yay':
play_game()
elif user_again == 'nay':
print "Ok bye! hope you enjoyed the game. See you soon! :)"
else:
print "Please choose correct option."
play_again()
def play_game():
computer_choice = choice(game_options)
computer_choosed = "Computer choosed %s" % computer_choice
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = raw_input('->')
if user_choice not in game_options:
print "please choose correct option"
play_game()
return
if user_choice == computer_choice:
print computer_choosed
print "So it's a tie"
elif user_choice == 'stone':
if computer_choice == 'paper':
print computer_choosed
print "So, You loose"
elif computer_choice == 'scissor':
print computer_choosed
print "So, Cheers! You won!"
elif user_choice == 'paper':
if computer_choice == 'scissor':
print computer_choosed
print "So, you loose."
elif computer_choice == 'stone':
print computer_choosed
print "So, Cheers! You won!"
elif user_choice == 'scissor':
if computer_choice == 'stone':
print computer_choosed
print "So, you loose."
elif computer_choice == 'paper':
print computer_choosed
print "So, Cheers! You won!"
play_again()
if __name__ == "__main__":
# execute only if run as a script
print "You are playing Stone, Paper, Scissor."
play_game()
Also, this may call for a better option. You could define a function asking a user for a value in a list. This function could be used in 2 places and make your code easier to follow and less repetitive.
from random import choice
game_options = ['stone', 'paper', 'scissor']
def get_user_input_in_list(lst):
user_input = raw_input('->')
while True:
if user_input in lst:
return user_input
else:
print "Please choose correct option."
def play_again():
print "Do you want to play again:- Choose 'yay' or 'nay'"
user_again = get_user_input_in_list(['yay', 'nay'])
if user_again == 'yay':
play_game()
elif user_again == 'nay':
print "Ok bye! hope you enjoyed the game. See you soon! :)"
def play_game():
computer_choice = choice(game_options)
computer_choosed = "Computer choosed %s" % computer_choice
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = get_user_input_in_list(game_options)
if user_choice == computer_choice:
print computer_choosed
print "So it's a tie"
elif user_choice == 'stone':
if computer_choice == 'paper':
print computer_choosed
print "So, You loose"
elif computer_choice == 'scissor':
print computer_choosed
print "So, Cheers! You won!"
elif user_choice == 'paper':
if computer_choice == 'scissor':
print computer_choosed
print "So, you loose."
elif computer_choice == 'stone':
print computer_choosed
print "So, Cheers! You won!"
elif user_choice == 'scissor':
if computer_choice == 'stone':
print computer_choosed
print "So, you loose."
elif computer_choice == 'paper':
print computer_choosed
print "So, Cheers! You won!"
play_again()
if __name__ == "__main__":
# execute only if run as a script
print "You are playing Stone, Paper, Scissor."
play_game()
This is better but we still have play_game
calling play_again
and play_again
calling play_game
.
Maybe play_game
should be used to play a single game and shouldn't call play_again
at all. This can be done by removing the call to play_again
in play_game
and moving it after the call to play_game
in play_again
. That way, we'd just have play_game
calling itself.
You'd have something like :
from random import choice
game_options = ['stone', 'paper', 'scissor']
def get_user_input_in_list(lst):
user_input = raw_input('->')
while True:
if user_input in lst:
return user_input
else:
print "Please choose correct option."
def play_again():
print "Do you want to play again:- Choose 'yay' or 'nay'"
user_again = get_user_input_in_list(['yay', 'nay'])
if user_again == 'yay':
play_game()
play_again()
elif user_again == 'nay':
print "Ok bye! hope you enjoyed the game. See you soon! :)"
def play_game():
computer_choice = choice(game_options)
computer_choosed = "Computer choosed %s" % computer_choice
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = get_user_input_in_list(game_options)
if user_choice == computer_choice:
print computer_choosed
print "So it's a tie"
elif user_choice == 'stone':
if computer_choice == 'paper':
print computer_choosed
print "So, You loose"
elif computer_choice == 'scissor':
print computer_choosed
print "So, Cheers! You won!"
elif user_choice == 'paper':
if computer_choice == 'scissor':
print computer_choosed
print "So, you loose."
elif computer_choice == 'stone':
print computer_choosed
print "So, Cheers! You won!"
elif user_choice == 'scissor':
if computer_choice == 'stone':
print computer_choosed
print "So, you loose."
elif computer_choice == 'paper':
print computer_choosed
print "So, Cheers! You won!"
if __name__ == "__main__":
# execute only if run as a script
print "You are playing Stone, Paper, Scissor."
play_game()
play_again()
This is a bit better but you can go further in the separation of concerns. It is probably a better option to have play_again
to return a boolean and have a while
loop ensuring we call play_game
as long as required.
This would look like:
from random import choice
game_options = ['stone', 'paper', 'scissor']
def get_user_input_in_list(lst):
user_input = raw_input('->')
while True:
if user_input in lst:
return user_input
else:
print "Please choose correct option."
def play_again():
print "Do you want to play again:- Choose 'yay' or 'nay'"
user_again = get_user_input_in_list(['yay', 'nay'])
return user_again == 'yay'
def play_game():
computer_choice = choice(game_options)
computer_choosed = "Computer choosed %s" % computer_choice
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = get_user_input_in_list(game_options)
if user_choice == computer_choice:
print computer_choosed
print "So it's a tie"
elif user_choice == 'stone':
if computer_choice == 'paper':
print computer_choosed
print "So, You loose"
elif computer_choice == 'scissor':
print computer_choosed
print "So, Cheers! You won!"
elif user_choice == 'paper':
if computer_choice == 'scissor':
print computer_choosed
print "So, you loose."
elif computer_choice == 'stone':
print computer_choosed
print "So, Cheers! You won!"
elif user_choice == 'scissor':
if computer_choice == 'stone':
print computer_choosed
print "So, you loose."
elif computer_choice == 'paper':
print computer_choosed
print "So, Cheers! You won!"
if __name__ == "__main__":
# execute only if run as a script
print "You are playing Stone, Paper, Scissor."
while True:
play_game()
if not play_again():
print "Ok bye! hope you enjoyed the game. See you soon! :)"
break
Now we can get into the internals of play_game
.
First, you could get rid of the various way where printing is repeated.
def play_game():
computer_choice = choice(game_options)
computer_choosed = "Computer choosed %s" % computer_choice
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = get_user_input_in_list(game_options)
print computer_choosed
if user_choice == computer_choice:
assert result == 0
print "So it's a tie"
else:
win = False # unused value
if user_choice == 'stone':
win = (computer_choice == 'scissor')
elif user_choice == 'paper':
win = (computer_choice == 'stone')
elif user_choice == 'scissor':
win = (computer_choice == 'paper')
if win:
print "So, Cheers! You won!"
else:
print "So, you loose."
There are various way to define who wins in a game of player/scissors/rock. You could define a dictionnary mapping the different combinations possible to the winner. I quite like using modulo arithmetic to find the result.
Final code looks like:
from random import choice
game_options = ['stone', 'paper', 'scissor']
def get_user_input_in_list(lst):
user_input = raw_input('->')
while True:
if user_input in lst:
return user_input
else:
print "Please choose correct option."
def play_again():
print "Do you want to play again:- Choose 'yay' or 'nay'"
user_again = get_user_input_in_list(['yay', 'nay'])
return user_again == 'yay'
def play_game():
computer_choice = choice(game_options)
computer_choosed = "Computer choosed %s" % computer_choice
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = 'stone' # get_user_input_in_list(game_options)
computer_idx = game_options.index(computer_choice)
user_idx = game_options.index(user_choice)
result = (computer_idx - user_idx) % 3
print computer_choosed
if result == 0:
print "So it's a tie"
elif result == 1:
print "So, you loose."
else:
assert result == 2
print "So, Cheers! You won!"
if __name__ == "__main__":
# execute only if run as a script
print "You are playing Stone, Paper, Scissor."
while True:
play_game()
if not play_again():
print "Ok bye! hope you enjoyed the game. See you soon! :)"
break
-
5\$\begingroup\$ I disagree with the final step. Using indices obfuscates the code and makes it unreadable. I'd consider writing another function and returning a custom Enum or a value in
True
(player wins),False
(comp wins),None
(tie) \$\endgroup\$WorldSEnder– WorldSEnder2016年07月13日 14:11:23 +00:00Commented Jul 13, 2016 at 14:11 -
\$\begingroup\$ @MathiasEttinger A typo introduced to see who was actually reading the code. (The real reason is some dirty testing code I forgot to cleanup, I've just fixed it, thanks for letting me know!) \$\endgroup\$SylvainD– SylvainD2016年07月13日 15:10:57 +00:00Commented Jul 13, 2016 at 15:10
-
\$\begingroup\$ oh, and same for the
get_user_input_in_list
commented a few lines latter ;) \$\endgroup\$301_Moved_Permanently– 301_Moved_Permanently2016年07月13日 15:17:44 +00:00Commented Jul 13, 2016 at 15:17 -
1\$\begingroup\$ I would add a note about string formatting- we should be using the newer
"{}".format()
syntax instead of % formatting (it's equally easy to understand and so much more flexible) \$\endgroup\$Delioth– Delioth2016年07月13日 19:14:20 +00:00Commented Jul 13, 2016 at 19:14 -
\$\begingroup\$ @Delioth Indeed (but I am guilty of doing this myself so I hardly detect it in other people's code) \$\endgroup\$SylvainD– SylvainD2016年07月13日 19:15:11 +00:00Commented Jul 13, 2016 at 19:15
Here is a go on getting rid of a lot of the repetition.
A side note: In English this game is traditionally called "Rock, Paper, Scissors"
Your functions SPC
and SPCa
are very similar, so I got rid of the latter.
The introduction of a dictionary beats
makes determining the winner a lot easier. It makes it also almost like reading English:
if computer_choice == beats[user_choice]
Splitting the code into responsibilities. Getting user input and determining the winner are separate concerns.
Use the if __name__ == '__main__':
syntax to only run the code when starting the program e.g. with $ python SPC.py
and not when importing from another script.
Both user input functions should be somewhat resilient against user code input. The function getting the user's choice will only finish and return if the choice is one of the defined ones. The play_again function will only return True if the user chooses y
, Y
, yay
or actually any string starting with a y
.
import random
beats = {'paper': 'scissors',
'rock': 'paper',
'scissors': 'rock'}
def get_user_input():
print "You are playing Rock, Paper, Scissors."
while True:
user_input = raw_input("Make a choice: {} ".format(beaten_by.keys()))
if user_input in beats:
return user_input
print "Invalid choice."
def play_again():
choice = raw_input("Do you want to play again? [y/n]: ")
try:
return choice.lower()[0] == 'y'
except (AttributeError, TypeError, IndexError):
return False
def RPS():
computer_choice = random.choice(beats.keys())
user_choice = get_user_input()
if user_choice == beats[computer_choice]:
print "{} beats {}, you win!".format(user_choice, computer_choice)
elif computer_choice == beats[user_choice]:
print "{} beats {}, computer wins!".format(computer_choice, user_choice)
else:
print "Tie."
if __name__ == '__main__':
while True:
RPS()
if not play_again():
break
Nice part about deriving all choices from the dictionary beats
: Implementing "Rock, Paper, Scissors, Lizard, Spock" means just adding the additional keys to beats
and changing it to contain sets instead of just a string:
beats = {'paper': {'scissors', 'lizard'},
'rock': {'paper', 'spock'},
'scissors': {'rock', 'spock'},
'lizard': {'scissors', 'rock'},
'spock': {'paper', 'lizard'}}
And in RPS
use in
instead of ==
:
if user_choice in beats[computer_choice]:
print "{} beats {}, you win!".format(user_choice, computer_choice)
elif computer_choice in beats[user_choice]:
print "{} beats {}, computer wins!".format(computer_choice, user_choice)
else:
print "Tie."
-
3\$\begingroup\$ I have to admit - very interesting use of dictionary \$\endgroup\$vakus– vakus2016年07月13日 09:50:17 +00:00Commented Jul 13, 2016 at 9:50
-
\$\begingroup\$ But what
if __name__ == '__main__':
does \$\endgroup\$Sanatan Chaudhary– Sanatan Chaudhary2016年07月13日 10:44:53 +00:00Commented Jul 13, 2016 at 10:44 -
2\$\begingroup\$ @SanatanChaudhary
'__main__'
is the name of the scope in which top-level code executes. A module’s__name__
is set equal to'__main__'
when read from standard input, a script, or from an interactive prompt. See here for slightly more info. \$\endgroup\$Graipher– Graipher2016年07月13日 10:58:31 +00:00Commented Jul 13, 2016 at 10:58 -
2\$\begingroup\$ This means if you invoke your script via
python SPC.py
it will run this code. If, in another python file, you doimport SPC
it will not run this code (but make the functions available to you asSPC.play_again()
, for example). \$\endgroup\$Graipher– Graipher2016年07月13日 11:00:15 +00:00Commented Jul 13, 2016 at 11:00 -
\$\begingroup\$ +1 just because someone finally mentioned that it's ROCK paper scissors... X_X \$\endgroup\$Idos– Idos2016年07月13日 11:34:56 +00:00Commented Jul 13, 2016 at 11:34
I did not changed much in your code, but made some optimizations and tweaks.
Function play_again
def play_again():
while True:
print "Do you want to play again:- Choose 'yay' or 'nay'"
user_again = raw_input('->').lower() #change user output to lowercase
if user_again == 'yay':
SPC()
elif user_again == 'nay':
print "Ok bye! hope you enjoyed the game. See you soon! :)"
break #exit from loop
else:
print "Please choose correct option."
#Print message and repeat loop
- I changed calling
play_again
if the input was incorrect towhile True
loop. I decided to do so because calling own function may cause crashes etc.
to present possible crash I made this code:
def test():
x = 'x'
if x == 'y':
print('yes')
elif x == 'n':
print('no')
else:
test()
it "simulates" your function with user always writing wrong answer. In one moment function crashes with RunTimeError
- I also made sure that the player input is turned to lowercase, so if user write
yay
no matter is it upper case or lower case it will be working as lower caseyay
(the same withnay
)
Function SPC
def SPC():
computer_choice = choice( ['stone', 'paper', 'scissor'] )
computer_choosed = "Computer choosed %s" % computer_choice
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = raw_input('->').lower()
while user_choice not in ['stone', 'paper', 'scissor']:
print "please choose correct option"
user_choice = raw_input('->').lower()
print computer_choosed
if user_choice == computer_choice:
print "So it's a tie"
elif user_choice == 'stone':
if computer_choice == 'paper':
print "So, You loose"
elif computer_choice == 'scissor':
print "So, Cheers! You won!"
elif user_choice == 'paper':
if computer_choice == 'scissor':
print "So, you loose."
elif computer_choice == 'stone':
print "So, Cheers! You won!"
elif user_choice == 'scissor':
if computer_choice == 'stone':
print "So, you loose."
elif computer_choice == 'paper':
print "So, Cheers! You won!"
play_again()
This function was mostly alright, but I still made some tweaks
- Print computers choice before if statements. In this case we dont have to write
print computer_choosed
in everyif
, making the code shorter (quicker to read by us) - bring user input to lowercase
- Deleted calling
play_again
after each check, and added it at the end of method. I did this, because thoseif
s wont be executed anyway, and this caused that we don't need write this function every 4th line which make file bigger. Check is user input alright:
user_choice = raw_input('->').lower() while user_choice not in ['stone', 'paper', 'scissor']: print "please choose correct option" user_choice = raw_input('->').lower()
What happens here is:
- User is asked for input
- Code goes to
while
loop, if user input is right, thenuser_choice not in ['stone', 'paper', 'scissor']
will result in beingFalse
which will cause the loop to never actually execute. If user input wont be right then loop will print message and ask for input again, until input is right
rest of code
- I have deleted function SCP and replaced it with SCPa, because it was just copy of SCPa except the
print "You are playing Stone, Paper, Scissor."
to make something that works exactly as your SCP function I printed the message before you are calling SCP function:
print "You are playing Stone, Paper, Scissor."
SCP()
Full code:
from random import choice
def play_again():
while True:
print "Do you want to play again:- Choose 'yay' or 'nay'"
user_again = raw_input('->').lower() #change user output to lowercase
if user_again == 'yay':
SCP()
elif user_again == 'nay':
print "Ok bye! hope you enjoyed the game. See you soon! :)"
break #exit from loop
else:
print "Please choose correct option."
#Print message and repeat loop
def SPC():
computer_choice = choice( ['stone', 'paper', 'scissor'] )
computer_choosed = "Computer choosed %s" % computer_choice
print "Make an choice"
print "Choose stone, paper, scissor"
user_choice = raw_input('->').lower()
while user_choice not in ['stone', 'paper', 'scissor']:
print "please choose correct option"
user_choice = raw_input('->').lower()
print computer_choosed
if user_choice == computer_choice:
print "So it's a tie"
elif user_choice == 'stone':
if computer_choice == 'paper':
print "So, You loose"
elif computer_choice == 'scissor':
print "So, Cheers! You won!"
elif user_choice == 'paper':
if computer_choice == 'scissor':
print "So, you loose."
elif computer_choice == 'stone':
print "So, Cheers! You won!"
elif user_choice == 'scissor':
if computer_choice == 'stone':
print "So, you loose."
elif computer_choice == 'paper':
print "So, Cheers! You won!"
play_again()
print "You are playing Stone, Paper, Scissor."
SCP()
Explore related questions
See similar questions with these tags.
to choose
ischose
and notchoosed
. It isa choice
, notan choice
. \$\endgroup\$if
loops \$\endgroup\$