9
\$\begingroup\$

I'd like to know how to improve this code because I think that the main_func is too big and I can not split it into other functions or classes.

Also, I want to know if I can or should use classes to make it cleaner.

import random
import string


def starting():
    print('HANGMAN')
    print('Set game mode:')
    print('0. To exit')
    print('1. Easy')
    print('2. Medium')
    print('3. Hard')
    difficult = int(input('Your choice: '))
    if difficult == 1:
        difficult_easy()
    elif difficult == 2:
        difficult_medium()
    elif difficult == 3:
        difficult_hard()

    else:
        exit('Exiting...')


def main_func(word_lst, guesses_given):

    secret_word = random.choice(word_lst)
    output = []
    guessed_letters = []
    alphabet = string.ascii_letters
    length = len(secret_word)
    print(f'Your word has {len(secret_word)} characters ')

    for i in range(len(secret_word)):
        output.append('_')

    while '_' in output:

        letter = input('Enter a letter: ')

        if letter not in alphabet:
            print('You should enter only one letter!\n ')
        elif len(letter) != 1:
            print('You can only display 1 letter at a time\n')
        else:
            if letter not in guessed_letters:
                guessed_letters.append(letter)

                if letter in secret_word:
                    for n in range(length):
                        if secret_word[n] == letter:
                            output[n] = letter.upper()
                    print(*output, sep=' ')
                    if '_' not in output:
                        print('You won!')

                if letter not in secret_word:
                    guesses_given -= 1
                    print(f"This letter is not in the secret word. REMAINING TRIES: {guesses_given}\n")
                    if guesses_given == 0:
                        print(f"You lost. The secret word was '{secret_word.upper()}'")
                        break

            else:
                print('You have already guessed this letter!\n\n')



    print('GAMEOVER')
    play_again()



def play_again():
    again = input('Play again? (y/n)\n')
    if again.lower() == 'yes' or again.lower() == 'y':
        starting()
    else:
        exit('Exiting...')


def difficult_easy():
    main_func(['hall', 'exam', 'road', 'gate', 'debt', 'poet', 'sir', 'girl', 'food'], 14)


def difficult_medium():
    main_func(['customer', 'baseball', 'language', 'stranger', 'quantity',
               'judgment', 'republic', 'proposal', 'magazine'], 12)


def difficult_hard():
    main_func(['assumption', 'impression', 'restaurant', 'indication', 'excitement',
               'depression', 'government', 'inspection', 'protection', 'investment'], 10)


if __name__ == '__main__':
    starting()

For me, it looks garbage, but I did my best to make it simple and short at the same time. I want to use classes to make it more simple, but I still hadn't figured out... The last thing I want to ask is about the if __name__ == '__main__':, am I using it correctly?

I posted it on Stack Overflow but people said that the site is only for specific problems in your code, so they told me to go here.

\$\endgroup\$
  • 1
    \$\begingroup\$ "that the site is only for specific problems in your code, so they told me to go here" Well, this site here is about improvement of code all implemented, and working as intended (to the authors best knowledge). So if you confirm that your code does everything as intended, you're right here. Bur if you're asking whether you do if __name__ == '__main__': correctly, I have some doubts. \$\endgroup\$ – πάντα ῥεῖ Aug 7 at 10:43
  • 2
    \$\begingroup\$ The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$ – Mast Aug 7 at 10:46
  • \$\begingroup\$ Yeah, i'm beggining to code, so this is all new for me, thanks \$\endgroup\$ – filip augusto Aug 7 at 16:58
  • 2
    \$\begingroup\$ Some years back I wrote a sample Hangman Game for a Python programming class I was teaching. You could read it for inspiration. It's not really that different from yours, conceptually. \$\endgroup\$ – bjmc Aug 7 at 19:57
  • \$\begingroup\$ sure, actually really similar, but i found yours more organized @bjmc \$\endgroup\$ – filip augusto Aug 7 at 21:54
2
\$\begingroup\$

Readability 1

You mentioned splitting the code into functions, but functions should also be meaningful.

I would remove the def difficult_easy(): functions, since they just call the main_func anyway, and put the contents of each of those functions directly in the if-else branch in the starting() function.

Like this:

if difficult == 1:
    main_func(['hall', 'exam', 'road', 'gate', 'debt', 'poet', 'sir', 'girl', 'food'], 14)

This makes the code more readable and shorter. These 3 functions don't add anything useful or readable. They force me to look at the bottom of the file to see what they do, when that code could be in the same place as the if-else branch.

Readability 2

if letter not in alphabet:
    print('You should enter only one letter!\n ')

I would add continue here on the line after print . It doesn't change the functionality, but it makes it clear that this is the end of the while loop in this branch and when reading the code I don't have to read further to see if anything more happens after the if-else branching. It also ensures that you don't accidentally execute code that you might add later on below the if-else branch.

Readability 3

if letter not in alphabet:
    print('You should enter only one letter!\n ')
elif len(letter) != 1:
    print('You can only display 1 letter at a time\n')

These "early exit" branches are nice and make the code more readable. You could make one more early exit in the same style, by moving the

if letter in guessed_letters:
    print('You have already guessed this letter!\n\n')

To come third here, instead of being nested at the very bottom. Logically, it doesn't change the program, but it becomes more readable and less nested, which is generally a good thing.

Use variables

You have defined the variable length = len(secret_word) but you're not using it, instead you are repeating len(secret_word) several times in the code that follows, where you could just use length.

Other 1

output = []

for i in range(len(secret_word)):
    output.append('_')

All of this can be replaced by just one line output = "_" * length since Python allows multiplying a string by a number. (It has to be below the definition of length)

https://docs.python.org/3/library/stdtypes.html#common-sequence-operations

Other 2 (edit: this suggestion is not valid, we do need the index for the output. )

for n in range(length):
    if secret_word[n] == letter:
        output[n] = letter.upper()

The above is a very C-style loop, but in Python you don't need to loop through the indexes, you can access the characters in the string directly like this:

for c in secret_word:
    if c == letter:
        output[n] = letter.upper()

Other 3 (added after posting)

if again.lower() == 'yes' or again.lower() == 'y':

To avoid repeating again.lower() , this can be changed into

if again.lower() in ['yes', 'y']:

When you have more than 2 options, this becomes even more useful.

Final comments

Regarding classes and functions, I don't think you need them. This program is small enough and readable that it would just become more complex if you add classes and functions. If you want to practice, I suggest writing a bigger program instead where they would come to good use.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you so much for the analisys, I tried fix all that u have said. I'm really surprised in how people answer it so fast and so complete, really impressive. I'm gonna post the final result soon \$\endgroup\$ – filip augusto Aug 7 at 23:06
  • \$\begingroup\$ Glad to help. It was not so bad to start with. Readable and structured code from the start made it easy to improve on. \$\endgroup\$ – user985366 Aug 7 at 23:19
  • \$\begingroup\$ I tried to remove the 'C' style loop but when i did it i got an error, so i'm using the range func again . Traceback (most recent call last): File "C:/Users/filip/PycharmProjects/HelloWorld/utils.py", line 77, in <module> set_gamemode() File "C:/Users/filip/PycharmProjects/HelloWorld/utils.py", line 24, in set_gamemode execute_game(random.choice(medium_words), 10) File "C:/Users/filip/PycharmProjects/HelloWorld/utils.py", line 54, in execute_game output[c] = letter_input.upper() TypeError: list indices must be integers or slices, not str \$\endgroup\$ – filip augusto Aug 7 at 23:25
  • 1
    \$\begingroup\$ @filipaugusto I made a mistake in my "Other 2" suggestion. You do need the index, since you are also modifying the letter in output at the same index. I forgot about that when suggesting the shorter version. \$\endgroup\$ – user985366 Aug 8 at 0:12
  • \$\begingroup\$ Ok, i'll fix it \$\endgroup\$ – filip augusto Aug 8 at 2:58
5
\$\begingroup\$

Whitespace Following Code Blocks

According to the Python style guide, you should use whitespace sparingly. Try aiming for one single line between functions and code blocks.

Unclear Function Naming

main_func() is not a very clear function name. As a developer reading the code, I am unsure as to what this function contains.

For a solution to this problem, read the next section:

Single-Responsibility Principle

Instead of grouping all of the main game code into main_func(), figure out blocks of code that have a single responsibility, and refactor them into their own function.

For example, the start of main_func() contains code to choose a word. You could refactor this into a choose_word() function that takes the list of words. From this point, you might choose to not pass word_lst into main_func, but instead the chosen word as a result of the choose_word() function.

As another example, further into your main_func() function, you may choose to refactor the "check" code (to see if the player has correctly guessed the word) into a check_guess() function.

Parameter Naming

I am not sure if this is just a typo or a stylistic choice, but you should rename word_lst to word_list. In this example, other developers (and possibly yourself in future) will be able to figure out that lst == list, but some words may not be so obvious. Try not to shorten words when naming variables parameters.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Ok thank you for your time, i really appreciate it. Right now I will try it out what u have said, then will post an update \$\endgroup\$ – filip augusto Aug 7 at 16:55
4
\$\begingroup\$

The biggest problem with this code is the way methods are used, in fact this causes a bug when you try to play more than 250 games.

Other than goto-statements that can be found in Basic and other languages, methods are usually doing a thing and then returning the control flow back to where they were called from.

def do_three_things()
    do_first_thing()
    do_second_thing()
    do_third_thing()

def do_first_thing()
    print("I")

def do_second_thing()
    print("II")

def do_third_thing()
    print("III")

In your code every methods ends either with calling exit or another method.

def do_three_things()
    do_first_thing()

def do_first_thing()
    print("I")
    do_second_thing()

def do_second_thing()
    print("II")
    do_third_thing()

def do_third_thing()
    print("III")

Readability is one problem with this:

If you look at the method do_three_things in the first example, you see that is very clear what "doing three things" means from the method, in the second case it only looks like is doing the first thing.

The bigger problem is when you use infinite recursion. That is after the game is over you call method starting all over again, while it actually is still being executed. This way the interpreter has to keep the context of the first method call in memory while the second one is executed, by the moment you play 250 games, it becomes too much for the interpreter and it while throw an exception.

The way to fix this is to not call play_again from main_func instead return to the main function:

def main_func(word_lst, guesses_given):
    secret_word = random.choice(word_lst)
    output = []
    guessed_letters = []
    alphabet = string.ascii_letters
    length = len(secret_word)
    print(f'Your word has {len(secret_word)} characters ')

    for i in range(len(secret_word)):
        output.append('_')

    while '_' in output:

        letter = input('Enter a letter: ')

        if letter not in alphabet:
            print('You should enter only one letter!\n ')
        elif len(letter) != 1:
            print('You can only display 1 letter at a time\n')
        else:
            if letter not in guessed_letters:
                guessed_letters.append(letter)

                if letter in secret_word:
                    for n in range(length):
                        if secret_word[n] == letter:
                            output[n] = letter.upper()
                    print(*output, sep=' ')
                    if '_' not in output:
                        print('You won!')

                if letter not in secret_word:
                    guesses_given -= 1
                    print(f"This letter is not in the secret word. REMAINING TRIES: {guesses_given}\n")
                    if guesses_given == 0:
                        print(f"You lost. The secret word was '{secret_word.upper()}'")
                        break

            else:
                print('You have already guessed this letter!\n\n')



    print('GAMEOVER')

Then you make play_again return a True or a False value depending on the choice made.

def play_again():
    again = input('Play again? (y/n)\n')
    if again.lower() == 'yes' or again.lower() == 'y':
        return True 
    else:
        return False

Now you can now have a loop in main that plays until the player has enough:

if __name__ == '__main__':
    starting()
    while play_again():
        starting()
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ yes that was something I didn't know for sure if I was it doing right. Right now i'm writting a code for a login system (simple as this hangman game) just wanna say thank you because I was doing the same infinite function loop, but after reading your review I now corrected it! \$\endgroup\$ – filip augusto Aug 10 at 23:09
1
\$\begingroup\$

That's the final result

I did not do much change but i think that now it is easier to read. I'm practising every day and I would love to know any other small project like this or bigger! When you started learning code, what small projects did you do? Also, if you know a good website for practising, please tell me in the coments.

import random
import string

alphabet = string.ascii_letters
easy_words = ['hall', 'exam', 'road', 'gate', 'debt', 'poet', 'sir', 'girl', 'food']
medium_words = ['customer', 'baseball', 'language', 'stranger', 'quantity',
                'judgment', 'republic', 'proposal', 'magazine']
hard_words = ['assumption', 'impression', 'restaurant', 'indication', 'excitement',
              'depression', 'government', 'inspection', 'protection', 'investment']

# Initialize the game
def set_gamemode():

    print('HANGMAN')
    print('To set the game mode, enter:')
    print('0. To exit')
    print('1. Easy')
    print('2. Medium')
    print('3. Hard')
    difficult = int(input('Your choice: '))
    if difficult == 1:
        execute_game(random.choice(easy_words), 12)
    elif difficult == 2:
        execute_game(random.choice(medium_words), 10)
    elif difficult == 3:
        execute_game(random.choice(hard_words), 9)
    else:
        exit('Exiting...')

# Main function that executes the game by its gamemode
def execute_game(word, guesses_given):

    guessed_letters = []
    length = len(word)
    output = ['_'] * length
    print(f'Your word has {length} characters ')

    while '_' in output:
        letter_input = input('Enter a letter: ')

        if letter_input not in alphabet:
            print('You should enter only one letter!\n ')
            continue
        elif len(letter_input) != 1:
            print('You can only display 1 letter at a time\n')
        elif letter_input in guessed_letters:
            print('You have already guessed this letter!\n\n')
        else:
            guessed_letters.append(letter_input)

            if letter_input in word:
                for c in range(length):
                    if word[c] == letter_input:
                        output[c] = letter_input.upper()
                print(*output, sep=' ')
                print('\n')
                if '_' not in output:
                    print('You won!')

            elif letter_input not in word:
                guesses_given -= 1
                print(f"This letter is not in the secret word. REMAINING TRIES: {guesses_given}\n")
                if guesses_given == 0:
                    print(f"You lost. The secret word was '{word.upper()}'")
                    break

    print('GAMEOVER')
    play_again()

# The name says it
def play_again():
    again = input('Play again? (y/n)\n')
    set_gamemode() if again.lower() in ['y', 'yes'] else exit('Exiting...')

# Driver code
if __name__ == '__main__':
    set_gamemode()

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ When I was first learning, I did codingbat.com/python and also some of the problems on projecteuler.net (good if you like math) \$\endgroup\$ – bjmc Aug 8 at 17:07
  • \$\begingroup\$ yes i like maths thank you, i'm gonna try it \$\endgroup\$ – filip augusto Aug 10 at 22:56

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.