6
\$\begingroup\$

I have just finished learning basic-intermediate Python subjects and wanted to test myself. This word-guessing game is one of the first programs I have written.

import random


def pick_random_word():
    word_list = ["python", "c", "java", "swift", "html", "css", "go", "ruby"]
    random_word = random.choice(word_list)
    return random_word


def make_word_classified(word):
    classified_list = ["_" for i in word]
    return classified_list


def guess():
    word = pick_random_word()
    classified_word = make_word_classified(word)
    print(*classified_word)
    total_attempts = 0

    while True:
        try:
            answer = input("Guess a letter (Write only one letter)>: ").lower()
            if len(answer) > 1:
                raise Exception
        except Exception:
            print("Only one letter at a time!")
            continue
        total_attempts += 1

        if total_attempts >= 7:
            print("Sorry but you lost!")
            try_again = input("Wanna play again? (write y or n) >: ")
            if try_again == 'y':
                guess()
            elif try_again == 'n':
                print("Goodbye!")
                quit()

        for i in range(len(word)):
                if answer == word[i]:
                    classified_word[i] = answer
                if "".join(classified_word) == word:
                    print("You won!")
                    quit()

        print(*classified_word, f"\nTotal attempts left: {7 - total_attempts}")


if __name__ == "__main__":
    guess()

So, what do you think? How can I make it better? What are my mistakes?

\$\endgroup\$
  • \$\begingroup\$ Note that you always lose here if the word is "javascript", since you -always- increment, and it has > 7 unique letters. \$\endgroup\$ – Gloweye Nov 6 at 12:23
  • \$\begingroup\$ @Gloweye You are right, fixing it. \$\endgroup\$ – Haliax Nov 6 at 12:26
  • \$\begingroup\$ @Haliax, can you declare word_list as constant? \$\endgroup\$ – RomanPerekhrest Nov 6 at 14:26
  • \$\begingroup\$ @RomanPerekhrest You mean as a tuple? I don't know how to set a constant list. \$\endgroup\$ – Haliax Nov 6 at 16:17
6
\$\begingroup\$

General Remarks

  • For the most part, this code is quite good if it's truly one of the first programs you've written. I've seen experienced coders with a University degree do much worse things. Congrats.
  • Avoid: while True:, catch Exception, raise Exception in the future. These should only be used in rare cases, and yours wasn't one.
  • KISS. That try..except block where you raise and catch an exception is just too complex for what can simply be achieved with an if statement.
  • Consider the control flow. There's a couple of bugs that I found because some of the conditions were incomplete. I've annotated and fixed some of them in the line-by-line review, and will put them rest as "exercises" at the end.
  • Avoid the use of quit() (or even sys.exit()) and opt for an early return instead.

Personal Remarks and Hints

  • Consider adopting type hinting. A small program like this is ideal to explore Python type hinting, and tools such as mypy can make your life a lot easier once you get the hang of it.
  • I would've liked to see some docstrings in your module and functions.
  • Variable names and functions names were okay for the most part, although there's room for improvement. Always make them indicative of the variable/function's purpose, and avoid confusion.
  • Consider the visibility of your functions, and mark functions that should not be visible outside of your module as "private" by prefixing them with an underscore. Although unnecessary for this code, it's good practice for the future.

Line-by-Line Review

Without further ado, here's a line-by-line review with some inline comments and personal suggestions for improvement I've made while reading your code.

import random

# EXERCISES:
# - What if I win, but I want to play another round?
# - What happens if I guess the same character twice? What *should* happen?
#   Maybe it's already okay.

# COMMENT: Putting this list in the `pick_random_word` function will constantly
# reconstruct the list, which is redundant. Instead, moving it to the global
# scope will only construct it once.
# NOTE: Putting variables into the global scope is also considered bad
# practice, but in this simple case there's not a lot wrong with it. There's
# other options that I won't go into detail on.
# COMMENT: You removed "javascript" because @Gloweye correctly pointed out a
# bug in your program. We'll instead fix this bug here.
_WORD_LIST = [
        "python", "c", "java", "swift", "html", "css", "go", "ruby",
        "javascript"]

# COMMENT: Magic constants are a bad practice to have in your source code. I've
# extracted the maximum attempts to a global variable, so if you want to
# provide more attempts at a later date, you just have to change this, and not
# search for the number in the code.
_MAX_FAILED_ATTEMPTS = 7


# COMMENT: I've prefixed all function definitions with an underscore. This is
# mostly just a convention to denote names that should be kept internal. In
# case of modules, this signifies a user of the module that this is not a
# function they should be concerned with. In your case, this is probably
# redundant, but it's good practice for the future.
def _pick_random_word():
    # COMMENT: Don't assign and immediately return, instead, return without
    # assignment. This makes your code clearer.
    return random.choice(_WORD_LIST)


def _make_word_classified(word):
    # COMMENT: Again, immediately return instead of assigning to a variable
    # first. Secondly, lists can be "multiplied". This replicates the contents,
    # just like your list comprehension did previously.
    # Note: Mind the brackets so it's a list, not a string. Strings can be
    # multiplied too, but are immutable so won't work for the remainder of the
    # code.
    return ["_"] * len(word)
    # return classified_list


# COMMENT: A better name would be in order here. Something along the lines of
# `play_guessing_game` would be better, but still not ideal. `guess` feels like
# the wrong name to me.
def guess():
    # COMMENT: Confusing variable names are one of my pet peeves, so I changed
    # `word` to `target_word`. This way, when writing the code, you won't get
    # confused.
    target_word = _pick_random_word()
    classified_word = _make_word_classified(target_word)
    print(*classified_word)
    # COMMENT: Let's count the how many attempts are left, since that's what
    # we use more often: In printing as well as (now) the condition of the loop
    attempts_left = _MAX_FAILED_ATTEMPTS

    # COMMENT: I don't like `while True:`, unless it's really necessary.
    # I've changed it to iterate with a condition on the number of attempts
    # instead. This will also simplify our loop body.
    # COMMENT: We could simplify this to `while attempts_left` and use the fact
    # that 0 is equivalent to `False`, but this is more explicit.
    while attempts_left > 0:
        # COMMENT: The `try..except` block is over-engineered, it could've
        # been done with a simple `if` statement.
        answer = input("Guess a letter (Write only one letter)>: ").lower()
        # COMMENT: What happens if I don't enter anything? Should it really be
        # counted as an attempt? Thus I check if there's exactly one character.
        if len(answer) != 1:
            print("Exactly one letter is expected!")
            # COMMENT: I like the use of `continue` instead of an `else` block.
            # Both are viable, but for a large `else` body it gets hard on the
            # eyes. Well done.
            continue
        # COMMENT: Before I forget: You raised and caught `Exception`. In the
        # future, create your own custom exceptions instead, or use a specific
        # exception that's already provided by Python. `Exception` is the
        # superclass of almost all exceptions in Python, and by catching
        # exceptions, you would've suppressed different errors as well, such
        # as `IndexError`, `KeyError`, `AttributeError`, `TypeError`, ...

        # COMMENT: We'll only increment the attempt counter on mistakes, so
        # that words of arbitrary length are possible.
        # total_attempts += 1

        # COMMENT: We don't have to check this anymore, it's already checked
        # in the loop condition. Instead. we'll move the handling of running
        # out of attempts to after the loop.
        # if total_attempts >= _MAX_ATTEMPTS:
        #     print("Sorry but you lost!")
        #     try_again = input("Wanna play again? (write y or n) >: ")
        #     if try_again == 'y':
        #         guess()
        #     elif try_again == 'n':
        #         print("Goodbye!")
        #         quit()

        attempt_correct = False
        # COMMENT: Use enumerate(word) rather than range(len(word)) to get both
        # the value and the index.
        for char_idx, target_char in enumerate(target_word):
            # I've reindented this code to be 4 spaces rather than 8. New
            # blocks should always have 4 spaces.
            if answer == target_char:
                classified_word[char_idx] = answer
                attempt_correct = True

        # We still need to decrement the attempt counter if the attempt was
        # incorrect. This is why we maintain a boolean and set it to True only
        # if the attempt is correct.
        if not attempt_correct:
            attempts_left -= 1

        # COMMENT: Let's move this out of that loop, so we only compare the
        # words once, rather than every time we access a character.
        # COMMENT: Instead of turning the classified word into a string, let's
        # instead check whether it still contains an underscore to check if
        # we're done. This is more elegant.
        if "_" not in classified_word:
            print("You won!")
            # COMMENT: Instead of calling `quit()`, we'll return. I'm
            # `quit()` is not really an elegant way to exit a program,
            # and is not necessary here. Returning early will simply
            # break out of the function (and thus also the loop) and
            # thus stop the game.
            # COMMENT: Exercise for you: What if I wanted to continue
            # to play another round?
            return

        # COMMENT: You could move this to the top of the loop, and do away
        # with the initial print before the loop, and then you'd have the
        # "Total attempts left" from the start.
        print(*classified_word, f"\nTotal attempts left: {attempts_left}")

    # If we reach the end of this loop, we've lost, since if we've won,
    # we'd already have returned from the function.
    print("Sorry but you lost!")
    try_again = input("Wanna play again? (write y or n) >: ")
    # COMMENT: Python makes no distinction between strings and characters, so
    # single quotes and double quotes are equivalent. "y" and "n" here were
    # single quotes while the rest of your strings are double quotes.
    # Be consistent. Choose one and stick with it.
    if try_again == "y":
        # Okay, but what if I play millions of times? It's likely never going
        # to be an issue, but if I play millions of times, this will cause a
        # stack overflow because of the recursion. Prefer iteration (with a
        # `for` or `while` loop) instead. Python does not have tail-call
        # optimization: https://stackoverflow.com/q/13591970/10973209
        guess()
    # We still need the `else` to print the goodbye, otherwise it would print
    # goodbye multiple times if we recursively call ourselves. I've changed it
    # to `else` so that it prints goodbye even if I didn't say 'n'.
    # This would previously cause a bug when it was still in the loop. If I
    # entered 'a', it would just continue the game and I'd have an infinite
    # number of attempts.
    else:
        print("Goodbye!")
        # Now that this is moved outside of the loop, we don't need to return
        # or quit anymore, the function will just end.


if __name__ == "__main__":
    guess()

Exercises

There's still room for improvement, and I'll give you a few pointers on where to start:

  • Consider the case where I'm forgetful, and I've typed the same character twice. How should this be handled? How is this currently handled? Is that okay?
  • I've won the game. Now what? I'd like to play again, please and thank you.
  • I already mentioned in the code the possibility of stack overflow because Python doesn't perform so-called tail-call optimisation. I doubt you'll ever run into issues with this program because of this, but it's still a good exercise to 1) look into what tail-call optimisation is and why your program could crash without it, and 2) fix the program so that it wouldn't have that problem.
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you so much for your time and interest! Much appreciated! \$\endgroup\$ – Haliax Nov 6 at 16:14

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.