I am a C#/.Net developer and have decided it is time to branch out and really learn Python. I started with a simple implementation of a Mastermind-esque game. Any comments about the code would be greatly appreciated. I already know the gameplay isn't perfect, but it was just a quick attempt. I also am aware that there isn't much user input checking, but again, quick attempt.
import random
class Game(object):
def run(self):
guesses = []
game_states = []
length = int(input("Choose length of code to break: "))
code = [random.randint(0, 9) for i in range(length)]
def take_guess():
correct_guess_length = False
while not correct_guess_length:
guess = input("Guess a number of length {0}: ".format(length))
if len(guess) != length:
print("Guesses must be a number of length {0}.".format(length))
else:
correct_guess_length = True
return guess
def check_guess(guess):
return all([x == y for x, y in zip(guess, code)])
def evaluate_guess(guess):
new_state = []
for pos, number in enumerate(guess):
if code[pos] == number:
new_state.append(str(number))
elif number in code:
new_state.append("O")
else:
new_state.append("X")
game_states.append(''.join([c for c in new_state]))
def print_game_state():
print()
for guess, state in zip(guesses, game_states):
print("{0}\t{1}".format(guess, state))
correct = False
while not correct:
guess = take_guess()
guesses.append(guess)
guess_as_ints = [int(c) for c in guess]
if not check_guess(guess_as_ints):
evaluate_guess(guess_as_ints)
print_game_state()
else:
print("You guessed the code was {0}! Congratulations!".format(''.join([str(i) for i in code])))
correct = True
if __name__ == '__main__':
game = Game()
game.run()
-
\$\begingroup\$ Why would you nest the all the functions in the function run in a class? \$\endgroup\$Simon– Simon2016年12月16日 02:59:22 +00:00Commented Dec 16, 2016 at 2:59
3 Answers 3
Organization
As Simon points out, you shouldn't put the list of functions inside of run
. It doesn't make much sense. Move them out into the class.
Use self
self
in python is equivalent to this
in C# and many other languages.
This:
guesses = []
game_states = []
length = int(input("Choose length of code to break: "))
code = [random.randint(0, 9) for i in range(length)]
Should be this:
def __init__(self):
self.guesses = []
self.game_states = []
self.code = [] # Add this later when you define length.
(Also, I would keep length
as a local variable defined in run
and then refer to len(self.code)
instead.)
I would advise you look at how Python classes work.
You can replace random.randint(0, 9)
with random.randrange(10)
.
Furthermore, once you pull the functions out of run
, things like:
def take_guess():
become:
def take_guess(self):
Naming
correct_guess_length = False
I expect a varaible with the name of length
have some sort of integer (maybe real?) value. A boolean? Not often.
You don't really need this anyway!
else:
correct_guess_length = True
Can just be:
else:
break
I would probably just get rid of that too and say:
else:
return guess
Same thing goes for:
correct = False
while not correct:
...
You can get rid of these and use a break
statement instead.
randrange
code = [random.randint(0, 9) for i in range(length)]
Is pretty good except, randrange
is a little more specialized for this:
code = [random.randrange(10) for _ in range(length)]
(Also, when you disregard a variable the convention is to use _
, not i
.)
__str__
Since you rap everything in a game object, maybe you should give it a string representation overloading the __str__
function, and just print out the string representation of itself. Move print_game_state
to __str__
.
-
\$\begingroup\$ He should replace
length
toself.length
if he puts the other functions directly in the class. Otherwise,run()
andtake_guess()
wouldn't use the same variable. It couldn't even run unless you definelength
intake_guess()
\$\endgroup\$Aemyl– Aemyl2016年12月16日 06:39:11 +00:00Commented Dec 16, 2016 at 6:39 -
\$\begingroup\$ @Aemyl: I think he should replace
length
altogether withlen(self.code)
. I mention this in the comment afterlength
but it gets chopped off a bit. \$\endgroup\$Dair– Dair2016年12月16日 06:40:10 +00:00Commented Dec 16, 2016 at 6:40 -
\$\begingroup\$ As far as I know,
len()
can't stand to the left of an allocation operator \$\endgroup\$Aemyl– Aemyl2016年12月16日 06:42:33 +00:00Commented Dec 16, 2016 at 6:42 -
\$\begingroup\$ And he defines
self.code
after the definition oflength
\$\endgroup\$Aemyl– Aemyl2016年12月16日 06:44:00 +00:00Commented Dec 16, 2016 at 6:44 -
1\$\begingroup\$ @Aemyl: You can take the length of
code
. Also,self.code
andlength
are both never changed.len(self.code) == length
\$\endgroup\$Dair– Dair2016年12月16日 06:45:17 +00:00Commented Dec 16, 2016 at 6:45
Your code is hard to test
The functions are inside the
run
function making it impossible to run (no pun intended) them standalone to verify their correctness.They rely on the state of the class, so even after extracting them you cannot run them on their own. Instead the code that only performs logic should have an input and give an output, independent of the outside world.
A class?
The game is composed of many different pure functions with just a hint of state-keeping (3-4 variables in all). I would write them individually top-level.
Let me show you one example:
def evaluate_guess(guess, code):
"""
>>> evaluate_guess("12345", "19543")
'1XO4O'
"""
new_state = []
for pos, number in enumerate(guess):
if code[pos] == number:
new_state.append(str(number))
elif number in code:
new_state.append("O")
else:
new_state.append("X")
return ''.join([c for c in new_state])
Now the code is very easy to test, you just trivially call the function with the 2 arguments from any place anytime and you immediately get back the result. The test is immediate to write (you can run it with doctest
) and doubles as documentation.
Simplification
Now we can simplify the code and immediately get to see if our correction caused an obvious bug.
def evaluate_guess(guess, code):
"""
>>> evaluate_guess("12345", "19543")
'1XO4O'
"""
return ''.join(g if g == c else ('O' if g in code else 'X') for g, c in zip(guess, code))
I avoided the temporary variable and unnecessary repetition of new_state.append
boiling the code down to the essential logic. Now the reader gets the meaning behind without losing himself on details.
i have found that if the answer contains duplicate digits the game will output that the digit is correct but in the wrong spot eg. if the answer is 2812 and is guessed the code returns 281o and continues instead of the win message which is quite a problem.
If you replace:
if code[pos] == number:
with:
if number in code and code[pos] == number:
that should fix the problem.