Python, Rock, Paper, Scissor

Recently i wanted to test my Python knowledge so I'm taking some challenges. I came across a Rock Paper Scissor challenge and I wanted to make it with less code and reuse the same code over and over. The code below is what I have written so far and it works great. I just want to get your opinions: what can i do better?

rules = {
'rock': {
'scissor': 'smashes'
},
'paper': {
'rock': 'packs'
},
'scissor': {
'paper': 'cuts'
}
}

player_one = input("P1 | Rock, Paper or Scissor: ").lower()
player_two = input("P2 | Rock, Paper or Scissor: ").lower()

def play(p1, p2):
if p1 in rules:
if p2 in rules[p1]:
print("{} {} {}".format(p1, rules[p1][p2], p2))

if p2 in rules:
if p1 in rules[p2]:
print("{} {} {}".format(p2, rules[p2][p1], p1))

if p1 not in rules or p2 not in rules:
print("Invalid input")

if __name__ == '__main__':
play(player_one, player_two)


Perform the validation first, to eliminate redundant conditionals.

def play(p1, p2):
if p1 not in rules or p2 not in rules:
print("Invalid input")
elif p2 in rules[p1]:
print("{} {} {}".format(p1, rules[p1][p2], p2))
elif p1 in rules[p2]:
print("{} {} {}".format(p2, rules[p2][p1], p1))

• Ah, you are absolutely right. This is what i was looking for, a nice upgrade to my code. Thanks! – Johnny Niklasson Apr 18 '19 at 2:00

Don't let your users give bad input. You could use a TUI framework such as Inquirer to guide your users to chose from a list.

Also you should avoid to put business code at the top level scope of your module. You should define player_one and player_two from a method. Code that is at the top level scope of your module is only ever executed once as it's not reexecuted when you import after the first import.