4
\$\begingroup\$

My teacher gave us this game to make. basically, the computer needs to generate randomly 5 letters and there would be 5 players if the player guesses the correct letter given by the computer It would print "good job" else "good luck next time", I would appreciate it if you could make my code a bit better.

from string import ascii_uppercase
from random import choice
randompc = []
user_answers = []

for i in range(5):
    randompc.append(choice(ascii_uppercase))


for i in range(5):
    user_input = str(
        input("User " + str(i+1)+" enter a character from A-Z:\n"))
    while user_input.isalpha() == False and len(user_input) > 0:
        user_input = str(
            input("User " + str(i+1)+" enter a character from A-Z:\n"))
    else:
        user_answers.append(user_input.upper())


def checkifcorrect():
    for i in range(len(user_answers)):
        if user_answers[i] == randompc[i]:
            print("Good Job user "+str(i+1))
        else:
            print("Good luck next time user "+str(i+1))
    print("The answers were: ")
    print(*randompc, sep=", ")


checkifcorrect()
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

As it is, your program is already in a pretty good shape. There is an error in the logic of the input part (what happens if the user enters a string longer than one character? what happens if they enter an empty string?), but apart from that, it's a good starting point.

But there are several improvements that you can do in order to make your code more idiomatic, to improve the readability, and to give it a more concise structure. Your program logic basically consists of three parts: an initialization, an input part, and an evaluation. You have a separate function for the last part (your function checkifcorrect()), but not for the first two parts. This is the first change that I'd recommend, and my answer is basically progressing through these three parts.

Initialization

Your initialization is fairly simple. All that needs to be done is that you need a list of five randomly chosen letters. You decided to use the choice() function for that, which does work. But there's an even more tailor-suited function for your use case: choices(lst, k), which creates a list of k choices from lst (with replacement, i.e. the same elements can be chosen more than once). Your initialization function will now contain of a single line of code:

def assign_letters():
    return choices(ascii_uppercase, k=5)

User input

There is a Python idiom that you will frequently encounter when a program requires a valid input from the user (but also in many other places). This idiom uses a while True: loop. In your case, it could look like this:

while True:
    user_input = input("Enter a letter: ")
    if len(user_input) == 1 and user_input.isalpha():
        break

The logic here basically reverses the logic of your current while loop. Currently, the loop is repeated for as long as the previous input is invalid. The while True: loop is repeated forever – unless it's explicitly left by the break instruction if the current input is valid. This results in a very strict regimen of what is considered a correct input, and it has the advantage that you don't need two more or less identical input() instructions.

When it comes to creating the input prompts, you may want to look into Python format strings. This is a subtype of strings that comes in handy whenever you want to create a string that embeds the value of a variable (or for that matter, the value of any evaluation). You create a format string are like any other strings, but you prepend the opening quotation mark by the letter f. Furthermore, at the point where you want your variable to appear, you insert the name of your variable enclosed in curly brackets. In your case, you can use the following line for input:

user_input = input(f"User {i+1} enter a character from A-Z:\n")

At runtime, the sequence "{i+1}" will automatically be replaced by the current value of i+1.

Evaluation

You already have a function checkifcorrect that evaluates the answers. It does its job, but there are a few improvements here as well. The first improvement concerns the name checkifcorrect. There is the Python Style Guide "PEP 8" that describes the style of well-formed Python scripts, including naming conventions. While these conventions are only recommendations, they are are chosen to improve the readability of Python scripts, and they are in very common use. So in order to help others read your scripts, it's always a good idea to stick to PEP8. For function names, the recommendation is to use lowercase words combined with underscore characters. So, PEP8 would recommend check_if_correct() as a name.

This also means that the variable name randompc is not really well-formed – I will use random_pc from now on. Speaking of which: the next improvement concerns variable handling. As it is, your function check_if_correct() accesses the global variables user_answers and random_pc. These variables are called "global" because they were not created within the function check_if_correct(), and they were not passed as arguments to the function call. This has a downside: your function will fail to work if these variables are not created beforehand.

In a small problem like your assignment, this won't matter much, but even in slightly bigger scripts you don't want your functions to rely on variables that are created elsewhere. Very generally speaking, global variables are useful, but they need to be used with care, and more often than not, you want to rely on local variables instead (this is something that beginners often need to get used to). So, instead of using the global variables user_answers and random_pc, you should add these variable names as arguments to the function definition:

def check_if_correct(user_answers, random_pc):

With this definition, you can pass any variable to your function regardless of the name of that variable in the global scope.

The next thing to improve would be to replace the + str(i+1) bits and use format strings again when you print the user numbers:

if user_answers[i] == random_pc[i]:
    print(f"Good Job user {i+1}")
else:
    print(f"Good luck next time user {i+1}")

The last improvement concerns printing the list random_pc. Your line print(*random_pc, sep=", ") is actually a pretty clever solution – using * to pass the elements of a list as arguments to a function is a very useful trick to know.

But if you want to combine the elements of a list into a single string, there's a more idiomatic way that works independently of the print function with the sep argument. Every string provides the method join(lst). This method returns a new string that uses the original string as a separator between each element of the list lst. So, in your case, if you want to print the elements of random_pc separated by ", ", you can use the following command:

print(", ".join(random_pc))

Granted, it's not really shorter or less complex than your command, but it's the more conventional way if list elements are to be joined in a string.

Overall structure

Now, you have three functions: assign_letters() which returns a list containing the randomly chosen letters per player, get_guesses() which returns a list containing the guesses of each player, and check_if_correct() that compares each guess to the randomly chosen letter. We can combine them into a single code block:

letters = assign_letters()
guesses = get_guesses()
check_if_correct(guesses, letters)

This code clearly reflects the program logic: first, letters are assigned, then, guesses are received, and then, the guesses and letters are checked for correctness.

With regard to the placement of this code block, there's another Python idiom that you'll find in virtually every non-trivial script: at the end of the main script, you will find the code block that contains every line of code that should be executed if the script is started. And this code block is wrapped into a somewhat cryptic if condition, like so:

if __name__ == "__main__":
    letters = assign_letters()
    guesses = get_guesses()
    check_if_correct(guesses, letters)

This works because unless the script was imported as a module, the internal variable __name__ will contain the string "__main__" as its value. Technically speaking, this is the way to identify the "top-level script environment", and there's a Stackoverflow question that will explain it in detail.

All in one place

So, here's the sum of all of my recommendations:

from string import ascii_uppercase
from random import choices


def assign_letters():
    return choices(ascii_uppercase, k=5)


def get_guesses():
    user_answers = []
    for i in range(5):
        while True:
            user_input = input(f"User {i+1} enter a character from A-Z:\n")
            if len(user_input) == 1 and user_input.isalpha():
                break
        user_answers.append(user_input.upper())
    return user_answers


def check_if_correct(user_answers, random_pc):
    for i in range(len(user_answers)):
        if user_answers[i] == random_pc[i]:
            print(f"Good Job user {i+1}")
        else:
            print(f"Good luck next time user {i+1}")
    
    print("The answers were: ")
    print(", ".join(random_pc))


if __name__ == "__main__":
    letters = assign_letters()
    guesses = get_guesses()
    check_if_correct(guesses, letters)

And I do agree with @zachary-vance that your teacher assigned you an incredibly stupid, un-fun game to code.

\$\endgroup\$
1
  • \$\begingroup\$ Sorry for the late response but really, this was so helpful I truly appreciate it. \$\endgroup\$
    – BackpainYT
    Oct 5, 2021 at 19:19
2
\$\begingroup\$

My first feedback is that this is an incredibly stupid, un-fun game. Not your fault, but come on here teachers. It's not that hard to make an actually fun game that beginners can write.

Please clarify whether you are using Python 2 or Python 3 when asking for review. I'm going to assume Python 3, since Python 2 is now more or less discontinued.

As far as the code, it's generally pretty easy to understand! You did a good job, and can turn it in as-is. If you wanted to know how to improve it, I would mostly say there are some small style changes you can make, and also you have a few logic errors if users don't type a letter.

First, take a look at:

for i in range(5):
    user_input = str(
        input("User " + str(i+1)+" enter a character from A-Z:\n"))
    while user_input.isalpha() == False and len(user_input) > 0:
        user_input = str(
            input("User " + str(i+1)+" enter a character from A-Z:\n"))
    else:
        user_answers.append(user_input.upper())
  • You repeatedly use i+1, but you never use i directly, so let's just change the loop to go 1..5 instead of 0..4.
  • 'i' is a bad name--let's call that player_number instead
  • It's generally considered a little nicer in modern python to use formatting strings, instead of concatenating strings.
  • Your while logic is wrong. If the user presses Enter, you skip them, which is not what you want to do. This will result in the wrong logic in the next step
  • input already returns a string (in Python 3, this changed from Python 2), so you don't need to call str on the result
  • Don't use the while-else loop as a general rule. It doesn't do what you think, and if the user presses enter and then gets prompted for a second guess, you're not appending it.
for player_num in range(1,6): # 1..5
    user_input = input("User {} enter a character from A-Z:\n".format(player_num))
    while not (user_input.isalpha() and len(user_input) == 1):
        user_input = input("Invalid, please try again. User {} enter a character from A-Z:\n".format(player_num))
    user_answers.append(user_input.upper())

The final section should similarly use format strings and player_num.

\$\endgroup\$
3
  • \$\begingroup\$ Actually, an additional tip--there were logic errors in this program. Test your programs by hand (both things that should work and things that shouldn't) to catch these yourself. \$\endgroup\$ Sep 17, 2021 at 21:26
  • 3
    \$\begingroup\$ It might not be the most inspiring task in the world, but I don't really see how that's relevant to a code review. Additionally, I don't think it's necessary to "clarify whether you are using Python 2 or Python 3 when asking for review" — python 3 is standard now, and its usage is perfectly obvious here by the fact that print is a function. You don't start reviewing OP's code until your third paragraph. \$\endgroup\$ Sep 18, 2021 at 11:23
  • \$\begingroup\$ Thank you so much I appreciate your help truly. \$\endgroup\$
    – BackpainYT
    Oct 5, 2021 at 19:20

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.