5
\$\begingroup\$

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()
\$\endgroup\$
1
  • \$\begingroup\$ Why would you nest the all the functions in the function run in a class? \$\endgroup\$
    – Simon
    Commented Dec 16, 2016 at 2:59

3 Answers 3

7
\$\begingroup\$

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__.

\$\endgroup\$
12
  • \$\begingroup\$ He should replace length to self.length if he puts the other functions directly in the class. Otherwise, run() and take_guess() wouldn't use the same variable. It couldn't even run unless you define length in take_guess() \$\endgroup\$
    – Aemyl
    Commented Dec 16, 2016 at 6:39
  • \$\begingroup\$ @Aemyl: I think he should replace length altogether with len(self.code). I mention this in the comment after length but it gets chopped off a bit. \$\endgroup\$
    – Dair
    Commented 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
    Commented Dec 16, 2016 at 6:42
  • \$\begingroup\$ And he defines self.code after the definition of length \$\endgroup\$
    – Aemyl
    Commented Dec 16, 2016 at 6:44
  • 1
    \$\begingroup\$ @Aemyl: You can take the length of code. Also, self.code and length are both never changed. len(self.code) == length \$\endgroup\$
    – Dair
    Commented Dec 16, 2016 at 6:45
2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.