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)
