9
\$\begingroup\$

I have recently written this little Bulls and Cows game in Python:

"""Play Bulls and Cows."""
import random
NUM_DIGITS = 5

class IllegalNumber(Exception):
    """An exception to raise when the class Num has an instance of an
    illegal number."""
    pass

class Num(object):
    """Represent a number that consists of different NUM_DIGITS digits.
    n -- a representation of the number (int, str or a
          sequence of digits)."""
    def __init__(self, n):
        self.num = tuple(int(d) for d in str(n)) if isinstance(n, (int, str)) \
            else tuple(int(d) for d in n)  # convert n into a tuple of digits
        if not self._is_legal():
            raise IllegalNumber

    def _is_legal(self):
        if len(self.num) != NUM_DIGITS:
            return False
        if len(set(self.num)) < NUM_DIGITS: # Does it contain equal digits?
            return False
        if self.num[0] == 0:
            return False
        if not all(0 <= d <= 9 for d in self.num):
            return False
        return True

    def __str__(self):
        return ''.join(str(d) for d in self.num)

    def compare(self, other):
        """If one number was A's secret number and the second one
        was B's guess, how many bulls and cows are there? Return
        them in a (bulls, cows) tuple representation.

        >>> Num(1234).compare(Num(5243)) ==> 1, 2
        """
        bulls, cows = 0, 0
        for i in xrange(NUM_DIGITS):
            if self.num[i] == other.num[i]:
                bulls += 1
            elif self.num[i] in other.num:
                cows += 1
        return bulls, cows

class ComputerPlayer(object):
    """An average computer player."""
    def __init__(self):
        self.possible_nums = []
        for num in xrange(10**(NUM_DIGITS-1), 10**NUM_DIGITS):
        # Iterate over the numbers that has NUM_DIGITS digits.
            try:
                self.possible_nums.append(Num(num))
            except IllegalNumber:
                pass
        self._secret_num = random.choice(self.possible_nums)

    def guess(self, check_func):
        """Guess a number and check it with the given function.
        The function gets a guess and returns the bulls and cows.
        Return True if the player won, and False otherwise.
        """
        try:
            guess_num = random.choice(self.possible_nums)
        except IndexError:
            # If self.possible_nums is empty, the opponent cheated or
            # mistaked
            return True  # If one player mistakes, his opponent wins
        bulls, cows = check_func(guess_num)
        if bulls == NUM_DIGITS:
            return True
        self.possible_nums = [n for n in self.possible_nums
                              if n.compare(guess_num) == (bulls, cows)]
          # The possible numbers that could be the opponent's secret number.
        return self.possible_nums == []  # If it's empty, the opponent cheated

    def check(self, guess_num):
        """Check the opponent's guess, and return the bulls and cows."""
        return self._secret_num.compare(guess_num)

class HumanPlayer(object):
    """Ask the user for a guess, show him the suitable bulls and cows
    for the guess, and ask him for suitable bulls and cows for the
    opponent's guess - let the user play Bulls and Cows."""

    def guess(self, check_func):
        """Ask the user for a guess, and check it with the
        given function, that gets the guess and returns
        the bulls and cows in a (bulls, cows) representation.
        Print the results to the user.
        Return True if the player won, and False otherwise.
        """
        while True:
            try:
                guess_num = Num(raw_input('Enter a guess: '))
            except IllegalNumber:
                print ("The number should consist of %d different digits"
                       "and the first digit can't be 0." % NUM_DIGITS)
                continue
            break
        bulls, cows = check_func(guess_num)
        if (bulls, cows) == (NUM_DIGITS, 0):
            print 'You won!'
            return True
        print 'You have {} bulls and {} cows.'.format(bulls, cows)
        print '-----'
        return False

    def check(self, num):
        """Check the opponent's guess, and return the bulls and cows."""
        # The messages are indented, to improve the readibility
        # for the user - he can easily distinguish between messages
        # from `guess` and messages from `check`
        print "\tYour opponent's guess is {}.".format(num)
        bulls, cows = int(raw_input('\tBulls: ')), int(raw_input('\tCows: '))
        if (bulls, cows) == (NUM_DIGITS, 0):
            print 'You lost!'
        return bulls, cows

def play(player1, player2):
    """Play a game between the two given Player classes.
    Each player class should contain `guess` and `check` methods.
    `check` will get the opponent's guess and return the suitable
    bulls and cows, and `guess` will get a function (the opponent's
    `check` method) and let the player guess a number and check it
    with the given function. `guess` will return True if the player
    won, and False otherwise.
    Return 1 if player1 won, and 2 if player2 won.
    """
    current_player, opponent = random.sample((player1(), player2()), 2)
    while not current_player.guess(opponent.check):
        current_player, opponent = opponent, current_player
    return {player1: 1, player2: 2}[current_player.__class__]

play(HumanPlayer, ComputerPlayer)

raw_input("Thanks for playing Bulls and Cows! ")

It works well (as far as I know), and I want it to be reviewed. What do you think about the code?

\$\endgroup\$
1
3
\$\begingroup\$

Your IllegalNumber is an exception, so should be called IllegalNumberException. It's also not needed since ValueError already occupies this niche.

You should break this onto multiple lines:

self.num = tuple(int(d) for d in str(n)) if isinstance(n, (int, str)) \
    else tuple(int(d) for d in n)  # convert n into a tuple of digits

as

if isinstance(n, (int, str)):
    self.num = tuple(int(d) for d in str(n))
else:
    self.num = tuple(int(d) for d in n)

although it's simpler to do:

if isinstance(n, (int, str)):
    n = str(n)

self.num = tuple(map(int, n))

In fact, the isinstance check doesn't need to include str.

I would change _is_legal to

digits = set(range(10))

return self.num[0] and len(self.num) == NUM_DIGITS == len(digits & set(self.num))

I would personally use map in __str__.

compare can use zip:

bulls, cows = 0, 0
for digit, other_digit in zip(self.num, other.num):
    if digit == other_digit:
        bulls += 1
    elif digit in other.num:
        cows += 1
return bulls, cows

You could even do this:

pos_eq = sum(map(eq, self.num, other.num))
set_eq = len(set(self.num) & set(other.num))

return pos_eq, set_eq - pos_eq

although maybe I'm getting ahead of myself.

ComputerPlayer.__init__ can generate mostly correct digits with:

for digits in permutations(range(10), NUM_DIGITS)

This gives a good speed improvement to the initial load. Since the load is still noticably slow, I suggest improving it with something like

    digitss = permutations(range(10), NUM_DIGITS)

    for digits in digitss:
        if digits[0]:
            break

    self.possible_nums = [digits] + map(Num, digitss)

and making the _is_legal call external so that this loop doesn't require it. I would also change the constructor to just

self.num = n

since permutations already returns tuples of digits. This would make HumanPlayer.guess start with

while True:
    inp = raw_input('Enter a guess: ')

    try:
        digits = map(int, inp)
    except ValueError:
        pass
    else:
        guess_num = Num(digits)
        if guess_num.is_legal():
            break

    print ("The number should consist of %d different digits "
           "and the first digit can't be 0." % NUM_DIGITS)

In ComputerPlayer.guess, you shouldn't need two checks to determine if the human cheated. Namely, the first try...except seems unneeded.

I like the use of a higher-order function with check_func. It's very elegant. However, don't abuse it; HumanPlayer should not be running the game; that logic should be in play.

It's unfortunate that you don't get an indication that you lost, or if you lost due to being caught cheating. This would be easier to sort out if more was in play and not in the respective classes.

You only use each method of Num in one place with these changes; it makes sense IMHO to make these functions on tuples of digits to remove the need to create classes. This makes things a little cleaner. It also allows one to speed up ComputerPlayer.__init__ further.

You don't validate the user's input in HumanPlayer.check. check also shouldn't be in the business of saying You lost!; that should be game logic.

Using random.sample to shuffle in-place is fancy but it isn't good. Just use random.shuffle on another line.

This all gives:

"""Play Bulls and Cows."""
import random
NUM_DIGITS = 5

from itertools import permutations
from operator import eq

def is_legal(digits):
    return digits[0] and len(digits) == NUM_DIGITS == len(set(range(10)) & set(digits))

def format_digits(digits):
    return ''.join(map(str, digits))

def bulls_and_cows(left, right):
    """If one number was A's secret number and the second one
    was B's guess, how many bulls and cows are there? Return
    them in a (bulls, cows) tuple representation.

    >>> bulls_and_cows((1, 2, 3, 4), (5, 2, 4, 3)) ==> 1, 2
    """
    pos_eq = sum(map(eq, left, right))
    set_eq = len(set(left) & set(right))

    return pos_eq, set_eq - pos_eq

def get_int(inp):
    while True:
        try:
            return int(raw_input(inp))
        except ValueError:
            pass

class ComputerPlayer(object):
    """An average computer player."""
    def __init__(self):
        self.possible_nums = list(permutations(range(10), NUM_DIGITS))

        minimum = self.possible_nums.index(tuple(range(1, NUM_DIGITS+1)))
        del self.possible_nums[:minimum]

        self._secret_num = random.choice(self.possible_nums)

    def guess(self, check_func):
        """Guess a number and check it with the given function.
        The function gets a guess and returns the bulls and cows.
        Return True if the player won, and False otherwise.
        """
        guess_num = random.choice(self.possible_nums)
        bulls, cows = check_func(guess_num)

        if bulls == NUM_DIGITS:
            return True

        # The possible numbers that could be the opponent's secret number.
        self.possible_nums = [n for n in self.possible_nums if bulls_and_cows(n, guess_num) == (bulls, cows)]

        # If it's empty, the opponent cheated
        return self.possible_nums == []

    def check(self, guess_num):
        """Check the opponent's guess, and return the bulls and cows."""
        return bulls_and_cows(self._secret_num, guess_num)

class HumanPlayer(object):
    """Ask the user for a guess, show him the suitable bulls and cows
    for the guess, and ask him for suitable bulls and cows for the
    opponent's guess - let the user play Bulls and Cows."""

    def guess(self, check_func):
        """Ask the user for a guess, and check it with the
        given function, that gets the guess and returns
        the bulls and cows in a (bulls, cows) representation.
        Print the results to the user.
        Return True if the player won, and False otherwise.
        """
        while True:
            inp = raw_input('Enter a guess: ')

            try:
                guess_num = map(int, inp)
            except ValueError:
                pass
            else:
                if is_legal(guess_num):
                    break

            print ("The number should consist of %d different digits "
                   "and the first digit can't be 0." % NUM_DIGITS)

        bulls, cows = check_func(guess_num)

        if (bulls, cows) == (NUM_DIGITS, 0):
            print 'You won!'
            return True

        print 'You have {} bulls and {} cows.'.format(bulls, cows)
        print '-----'
        return False

    def check(self, num):
        """Check the opponent's guess, and return the bulls and cows."""
        # The messages are indented, to improve the readibility
        # for the user - he can easily distinguish between messages
        # from `guess` and messages from `check`
        print "\tYour opponent's guess is {}.".format(format_digits(num))
        bulls, cows = get_int('\tBulls: '), get_int('\tCows: ')

        if (bulls, cows) == (NUM_DIGITS, 0):
            print 'You lost!'

        return bulls, cows

def play(player1, player2):
    """Play a game between the two given Player classes.
    Each player class should contain `guess` and `check` methods.
    `check` will get the opponent's guess and return the suitable
    bulls and cows, and `guess` will get a function (the opponent's
    `check` method) and let the player guess a number and check it
    with the given function. `guess` will return True if the player
    won, and False otherwise.
    Return 1 if player1 won, and 2 if player2 won.
    """
    players = [player1(), player2()]
    random.shuffle(players)

    current_player, opponent = players
    while not current_player.guess(opponent.check):
        current_player, opponent = opponent, current_player

    return {player1: 1, player2: 2}[current_player.__class__]

play(HumanPlayer, ComputerPlayer)

raw_input("Thanks for playing Bulls and Cows! ")
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the review! However, HumanPlayer.guess and HumanPlayer.check still print You lost! and You won! in the final code - you didn't change that. I think the play function has to call a HumanPlayer.end_of_game (along with ComputerPlayer.end_of_game) method (with a boolean parameter for win/lose) that will print the messages - ComputerPlayer.end_of_game will just pass. What do you think? \$\endgroup\$
    – MrHezi
    Jan 5 '15 at 17:57
  • \$\begingroup\$ @Avnerium "you didn't change that" → Yes, I didn't really want to make even larger changes myself. Your solution seems great: +1. \$\endgroup\$
    – Veedrac
    Jan 9 '15 at 11:46
4
\$\begingroup\$
  • It look like a common behaviour you want a Player to have is the ability of def check(self, num): and def guess(self, check_func):. Consider adding an ABC to this code class Player with the 2 @abstractmethods and have HumanPlayer and ComputerPlayer inherit from this.

  • Move all the try and expect logic into a separate method (consider moving it in the class Player from the comment above if you decide to go that way), and wrap all you raw_input calls with this method. The idea being keep looking until you get a valid input from the user (youre' skipping this logic in bulls, cows = int(raw_input('\tBulls: ')), int(raw_input('\tCows: ')) .. which will just crash with invalid input)

Larger Note : I've been thinking the code in 5 sections (maybe reflecting hte code in these sections might make it more readable?):

  1. NumberValidation: the class solely responsible for running all the raw_input and validating the input until we get one (no one outside of this should be calling raw_input or running validation on the input values .. the game is "stuck" at this until a valid input is recognized)
  2. GameValidation: the class responsible for checking the state of the game and calling it complete once we have all bulls (a lot of the compare and some of the guess functionality can go in here)
  3. Player: the ABC class that represents what a player should look like with the proper default behavior
  4. The HumanPlayer and the ComputePlayer class implementations
  5. The main method for game play between HumanPlayer and the ComputerPlayer.
\$\endgroup\$
2
\$\begingroup\$

You could remove duplicates logic in

self.num = tuple(int(d) for d in str(n)) if isinstance(n, (int, str)) else tuple(int(d) for d in n)

By writing :

self.num = tuple(int(d) for d in (str(n) if isinstance(n, (int, str)) else n))
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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