7
\$\begingroup\$

Bulls and Cows is a simple code-breaking game sometimes better known as "Mastermind." The rules are explained here: https://en.wikipedia.org/wiki/Bulls_and_Cows

I would appreciate some feedback on the following implementation in terms of overall code quality, layout and logic. It's meant to be at a fairly basic level so I don't want to over complicate things, but want to avoid any obviously bad choices. Suggestions for improvements very welcome.

import random

SECRET_NUMBER_SIZE = 2
MAX_GUESSES = 5

# Generate secret number with SECRET_NUMBER_SIZE digits and no duplicates
secret_number = ""
while len(secret_number) < SECRET_NUMBER_SIZE:
    new_digit = str(random.randint(0, 9))
    if new_digit not in secret_number:
        secret_number += new_digit

# print(f"For testing. Secret number is: {secret_number}")

print(f"Guess my number. It contains {SECRET_NUMBER_SIZE}\
unique digits from 0-9")

remaining_turns = MAX_GUESSES

while remaining_turns <= MAX_GUESSES:

    # Get user guess and validate length
    player_guess = input("Please enter your guess: ")
    if len(player_guess) != SECRET_NUMBER_SIZE:
        print(f"Your guess must be {SECRET_NUMBER_SIZE} digits long.")
        continue

    # Main game logic
    if player_guess == secret_number:
        print("Yay, you guessed it!")
        break
    else:
        bulls = 0
        cows = 0
        for i in range(SECRET_NUMBER_SIZE):
            if player_guess[i] == secret_number[i]:
                bulls += 1
        for j in range(SECRET_NUMBER_SIZE):
            if player_guess[j] in secret_number and \
              player_guess[j] != secret_number[j]:
                cows += 1
        print(f"Bulls: {bulls}")
        print(f"Cows: {cows}")
        remaining_turns -= 1
        if remaining_turns < 1:
            print("You lost the game.")
            break
\$\endgroup\$
9
\$\begingroup\$

@Josay's and @Reinderien's answers both give useful advice, with @Josay's being particularly comprehensive. However, there is one point that I've noticed neither of them have covered

Generation of the secret number

# Generate secret number with SECRET_NUMBER_SIZE digits and no duplicates
secret_number = ""
while len(secret_number) < SECRET_NUMBER_SIZE:
    new_digit = str(random.randint(0, 9))
    if new_digit not in secret_number:
        secret_number += new_digit

This is an inelegant way of generating the secret number for two reasons:

  1. You concatenate to a string, an immutable type. This is much more expensive than necessary, which does not a notable impact on a program of this size, but could become problematic as you scale up; it's better to practice good techniques, even at small scales. Additionally, you should never need to iteratively concatenate to a string in situations where the string's contents are generated all at once, because there are two successively more efficient ways:

    • Since Python uses dynamic lists, you can create a list with the string's contents, and then use "".join(str(i) for i in src_lst)
    • To improve the efficiency even further, you can utilize the fact that you know the list's size already, and "pre-allocate" the list using lst = [None] * SECRET_NUMBER_SIZE and then iteratively fill in its contents using a numerical index variable. This is a bit unpythonic, as @Josay goes over in their answer, and is probably a bit of premature optimization, but it's a good trick to have up your sleave if you want to eek out a bit more performance, and may be extremely useful depending on the situation.
    • But there's an even better way, which we'll get to in a second...
  2. Every number is checked to ensure it hasn't already occurred in the result list. The reason this is not ideal is because at larger scales, one could end up generating many numbers before they stumble upon one that hasn't already been chosen. To mitigate this, one could lower the generation range each time, and offset the numbers accordingly, but it's probably not worth the effort because this isn't much of a practical concern, even at larger scales; generating random numbers using the Mersenne Twister is very cheap. But this concern helps lead to an elegant solution...

random.sample() is your friend here. It allows you to generate multiple distinct random numbers quite easily; all you have to provide is a population sequence and the size of the sample. For this situation, a population sequence is quite simple: range(10) supplies all the numbers between 0 and 9 inclusive. You can now simplify your 5 lines above into ~1 line:

secret_number = "".join((str(i) for i in
    random.sample(range(10), SECRET_NUMBER_SIZE)))
\$\endgroup\$
  • \$\begingroup\$ What about secret_number = ''.join(map(str, random.sample(range(10), SECRET_NUMBER_SIZE)))? Also note that your version used generator expressions, not lists. \$\endgroup\$ – Solomon Ucko Dec 27 '18 at 20:36
  • 1
    \$\begingroup\$ @SolomonUcko that is also a good solution, and probably a bit faster; I just stuck with generator expressions because they're a bit more versatile and it's my general habit, but map does work well in situations like this. Regarding the list comment, I never claimed my final solution used lists; I just used lists as an example during the explanation of my thought process. I think you may be referring to when I initially mentioned the join operator, but in that context, src_lst was the list in question. I could be a bit more explicit on how list comprehensions work, though. \$\endgroup\$ – Graham Dec 27 '18 at 20:52
6
\$\begingroup\$

Your code looks good and uses modern techniques like f-strings which is a nice touch. Let's see can be improved.

Warning: I did not know the game Bulls and Cows until I read your question so I won't be able to judge on that part.

Small functions

You should try to split your code into small functions.

The most obvious one could be:

def generate_secret_number(length):
    """Generate secret number with `length` digits and no duplicates."""
    secret_number = ""
    while len(secret_number) < length:
        new_digit = str(random.randint(0, 9))
        if new_digit not in secret_number:
            secret_number += new_digit
    return secret_number

Another one could be:

def compute_cows_and_bulls(player_guess, secret_number):
    """Return the tuple (cows, bulls) for player_guess."""
    bulls = 0
    cows = 0
    for i in range(SECRET_NUMBER_SIZE):
        if player_guess[i] == secret_number[i]:
            bulls += 1
    for j in range(SECRET_NUMBER_SIZE):
        if player_guess[j] in secret_number and \
          player_guess[j] != secret_number[j]:
            cows += 1
    return cows, bulls

Then, we can try to improve these independently

Improving generate_secret_number

PEP 8, the Python Code Style Guide suggests:

For example, do not rely on CPython's efficient implementation of in-place string concatenation for statements in the form a += b or a = a + b. This optimization is fragile even in CPython (it only works for some types) and isn't present at all in implementations that don't use refcounting. In performance sensitive parts of the library, the ''.join() form should be used instead. This will ensure that concatenation occurs in linear time across various implementations.

It really doesn't matter in your case because we use a length of 2 but it is also a good chance to take nice habits. We could write:

def generate_secret_number(length):
    """Generate secret number with `length` digits and no duplicates."""
    digits = []
    while len(digits) < length:
        new_digit = str(random.randint(0, 9))
        if new_digit not in digits:
            digits.append(new_digit)
    return "".join(digits)

Also, we could one again split the logic into smaller function. As we do so, we stop checking the length of digits when we do not change it

def generate_new_digit_not_in(lst):
    while True:
        d = str(random.randint(0, 9))
        if d not in lst:
            return d

def generate_secret_number(length):
    """Generate secret number with `length` digits and no duplicates."""
    digits = []
    while len(digits) < length:
        digits.append(generate_new_digit_not_in(digits))
    return "".join(digits)

Now, because we know the number of iterations, instead of using a while loop, we could use a for loop:

def generate_new_digit_not_in(lst):
    while True:
        d = str(random.randint(0, 9))
        if d not in lst:
            return d

def generate_secret_number(length):
    """Generate secret number with `length` digits and no duplicates."""
    digits = []
    for _ in range(length):
        digits.append(generate_new_digit_not_in(digits))
    return "".join(digits)

Improving compute_cows_and_bulls

First detail is that we could use i for both loops:

for i in range(SECRET_NUMBER_SIZE):
    if player_guess[i] == secret_number[i]:
        bulls += 1
for i in range(SECRET_NUMBER_SIZE):
    if player_guess[i] in secret_number and \
      player_guess[i] != secret_number[i]:
        cows += 1

Then, and more importantly, I highly recommend Ned Batchelder's talk "Loop like a native".

One of the key hindsight is that when you are given iterable(s), you usually do not need to use index access and when you don't need it, you should avoid it.

Thus, usually, when you write for i in range(list_length), you can do things in a better way as Python offers many tools to work on iterables.

Here, we need to index to iterate over 2 lists in the same time. We could use zip and get something like:

def compute_cows_and_bulls(player_guess, secret_number):
    """Return the tuple (cows, bulls) for player_guess."""
    bulls = 0
    cows = 0
    for p, s in zip(player_guess, secret_number):
        if p == s:
            bulls += 1
    for p, s in zip(player_guess, secret_number):
        if p in secret_number and p != s:
            cows += 1
    return cows, bulls

Then, we could decide to have a single loop:

def compute_cows_and_bulls(player_guess, secret_number):
    """Return the tuple (cows, bulls) for player_guess."""
    bulls = 0
    cows = 0
    for p, s in zip(player_guess, secret_number):
        if p == s:
            bulls += 1
        elif p in secret_number:
            cows += 1
    return cows, bulls

However, the previous version of the code is better if we want to introduce more helpful tools.

We could use the sum builtin to write for instance:

def compute_cows_and_bulls(player_guess, secret_number):
    """Return the tuple (cows, bulls) for player_guess."""
    bulls = sum(1 for p, s in zip(player_guess, secret_number) if p == s)
    cows = sum(1 for p, s in zip(player_guess, secret_number) if p in secret_number and p != s)
    return cows, bulls

Or the equivalent:

def compute_cows_and_bulls(player_guess, secret_number):
    """Return the tuple (cows, bulls) for player_guess."""
    bulls = sum(p == s for p, s in zip(player_guess, secret_number))
    cows = sum(p in secret_number and p != s for p, s in zip(player_guess, secret_number))
    return cows, bulls

Also, for cows we could try to be more clever and realise that instead of checking p != s, we could include them and then at the end substract bulls from cows.

def compute_cows_and_bulls(player_guess, secret_number):
    """Return the tuple (cows, bulls) for player_guess."""
    bulls = sum(p == s for p, s in zip(player_guess, secret_number))
    cows = sum(p in secret_number for p in player_guess)
    return cows - bulls, bulls

Note: in order to test my changes, I wrote the following tests:

assert compute_cows_and_bulls("10", "23") == (0, 0)
assert compute_cows_and_bulls("10", "13") == (0, 1)
assert compute_cows_and_bulls("10", "31") == (1, 0)
assert compute_cows_and_bulls("10", "01") == (2, 0)
assert compute_cows_and_bulls("10", "10") == (0, 2)

Indeed, one of the benefits of writing small functions with a well-defined behavior is that you can easily write unit-tests for these. In a more serious context, you'd use a proper unit-test frameworks.

Improving the main loop

Here again, we can reuse the techniques seen previously.

For instance, we could have a smaller function to handle and validate user input.

def get_user_guess(length):
    """Get user guess and validate length."""
    while True:
        player_guess = input("Please enter your guess: ")
        if len(player_guess) == length:
            return player_guess
        print(f"Your guess must be {length} digits long.")

Then realise that we never exit the main loop because of the while remaining_turns <= MAX_GUESSES condition: we could simply write while True - the game always ends on either a victory or a defeat.

Also, instead of counting the number of remaining turns, we could count the number of turns like this:

turns = 0
while True:
    turns += 1
    player_guess = get_user_guess(SECRET_NUMBER_SIZE)

    # Main game logic
    if player_guess == secret_number:
        print("Yay, you guessed it!")
        break
    cows, bulls = compute_cows_and_bulls(player_guess, secret_number)
    print(f"Bulls: {bulls}")
    print(f"Cows: {cows}")
    if turns >= MAX_GUESSES:
        print("You lost the game.")
        break

But then maybe we could reuse a simple for loop here.

Final code

Taking into account everything, we end up with:

import random


def generate_new_digit_not_in(lst):
    """Generate a random digit not in `lst`."""
    while True:
        d = str(random.randint(0, 9))
        if d not in lst:
            return d


def generate_secret_number(length):
    """Generate secret number with `length` digits and no duplicates."""
    digits = []
    for _ in range(length):
        digits.append(generate_new_digit_not_in(digits))
    return "".join(digits)


def compute_cows_and_bulls(player_guess, secret_number):
    """Return the tuple (cows, bulls) for player_guess."""
    bulls = sum(p == s for p, s in zip(player_guess, secret_number))
    cows = sum(p in secret_number for p in player_guess)
    return cows - bulls, bulls


def get_user_guess(length):
    """Get user guess and validate length."""
    while True:
        player_guess = input("Please enter your guess: ")
        if len(player_guess) == length:
            return player_guess
        print(f"Your guess must be {length} digits long.")


def play_game(secret_number_len, nb_guesses):
    secret_number = generate_secret_number(secret_number_len)
    print(f"For testing. Secret number is: {secret_number}")
    print(f"Guess my number. It contains {secret_number_len} unique digits from 0-9")

    for t in range(nb_guesses):
        player_guess = get_user_guess(secret_number_len)

        # Main game logic
        if player_guess == secret_number:
            print("Yay, you guessed it!")
            return
        cows, bulls = compute_cows_and_bulls(player_guess, secret_number)
        print(f"Bulls: {bulls}")
        print(f"Cows: {cows}")
    print("You lost the game.")


def unit_test_compute_cows_and_bulls():
    assert compute_cows_and_bulls("10", "23") == (0, 0)
    assert compute_cows_and_bulls("10", "13") == (0, 1)
    assert compute_cows_and_bulls("10", "31") == (1, 0)
    assert compute_cows_and_bulls("10", "01") == (2, 0)
    assert compute_cows_and_bulls("10", "10") == (0, 2)


if __name__ == '__main__':
    SECRET_NUMBER_SIZE = 2
    MAX_GUESSES = 50
    play_game(SECRET_NUMBER_SIZE, MAX_GUESSES)
\$\endgroup\$
  • 1
    \$\begingroup\$ Interesting strategy of iteratively editing your answer to add more content. Do you do this often, and do you find it useful for generating cohesive answers? In the past I've written long answers all at once, because it helps me to prioritize the most important points to the beginning (though admittedly, I am imperfect), while recently, I've been trending towards shorter answers that fill in the gaps for other answers. For a question like this though, there's not too many deep structural levels to analyze, so I can imagine this iterative approach working out. \$\endgroup\$ – Graham Dec 27 '18 at 16:13
  • 2
    \$\begingroup\$ @Graham It is not a real strategy as per se. Just that I sometime come here to fill small periods of free time (compilations, tests, etc). As for the cohesion of the answer, it is not my strong point. I usually do whatever I can to get a better understanding of the code (tests, refactoring) then edit things little by little. Going step by step can be interesting for the OP as it shows how to improve pieces of code that may not end up in the final code. \$\endgroup\$ – Josay Dec 27 '18 at 16:33
3
\$\begingroup\$

Looks quite clean. I can only see two things:

  • Create a main function to pull your code out of global scope
  • Do some list comprehension sums.

This:

for i in range(SECRET_NUMBER_SIZE):
    if player_guess[i] == secret_number[i]:
        bulls += 1

can be:

bulls = sum(1 for p, s in zip(player_guess, secret_guess)
            if p == s)

Similarly, this:

for j in range(SECRET_NUMBER_SIZE):
    if player_guess[j] in secret_number and \
        player_guess[j] != secret_number[j]:
            cows += 1

can be:

cows = sum(1 for p, s in zip(player_guess, secret_number)
           if p != s and p in secret_number)
\$\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.