3
\$\begingroup\$

I want to change (for better performance) or optimize my code, especially for and while loops, maybe with lambdas or list comprehensions.

def getAvailableLetters(lettersGuessed):
    '''
    lettersGuessed: list, what letters have been guessed so far
    returns: string, comprised of letters that represents what letters have not
      yet been guessed.
    '''
    alphabet = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']
    alphabet2 = alphabet[:]

    def removeDupsBetter(L1, L2):
        L1Start = L1[:]
        for e in L1:
            if e in L1Start:
                L2.remove(e)
        return ''.join(str(e) for e in L2)

    return removeDupsBetter(lettersGuessed, alphabet2)


def hangman(secretWord):
    '''
    secretWord: string, the secret word to guess.

    Starts up an interactive game of Hangman.

    * At the start of the game, let the user know how many 
      letters the secretWord contains.

    * Ask the user to supply one guess (i.e. letter) per round.

    * The user should receive feedback immediately after each guess 
      about whether their guess appears in the computers word.

    * After each round, you should also display to the user the 
      partially guessed word so far, as well as letters that the 
      user has not yet guessed.

    Follows the other limitations detailed in the problem write-up.
    '''
    intro = str(len(secretWord))
    lettersGuessed = []
    guess = str
    mistakesMade = 8
    wordGuessed = False

    print 'Welcome to the game, Hangman!'
    print ('I am thinking of a word that is ') + intro + (' letters long.')
    print ('------------')

    while mistakesMade > 0 and mistakesMade <= 8 and wordGuessed is False:
        if secretWord == getGuessedWord(secretWord, lettersGuessed):
            wordGuessed = True
            break
        print ('You have ') + str(mistakesMade) + (' guesses left.')
        print ('Available letters: ') + getAvailableLetters(lettersGuessed)
        guess = raw_input('Please guess a letter: ').lower()
        if guess in secretWord:
            if guess in lettersGuessed:
                print ("Oops! You've already guessed that letter: ") + getGuessedWord(secretWord, lettersGuessed)
                print ('------------')
            else:
                lettersGuessed.append(guess)
                print ('Good guess: ') + getGuessedWord(secretWord, lettersGuessed)
                print ('------------')
        else:
            if guess in lettersGuessed:
                print ("Oops! You've already guessed that letter: ") + getGuessedWord(secretWord, lettersGuessed)
                print ('------------')
            else:
                lettersGuessed.append(guess)
                mistakesMade -= 1
                print ('Oops! That letter is not in my word: ') + getGuessedWord(secretWord, lettersGuessed)
                print ('------------')

    if wordGuessed == True:
        return 'Congratulations, you won!'
    elif mistakesMade == 0:
        print ('Sorry, you ran out of guesses. The word was ') + secretWord
\$\endgroup\$
  • \$\begingroup\$ You seem to be missing the definition of the function getGuessedWord. \$\endgroup\$ – Graipher Apr 16 '17 at 18:59
  • \$\begingroup\$ Note that the following are equivalent: alphabet = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'] ; alphabet_easier = list("abcdefghijklmnopqrstuvwxyz") \$\endgroup\$ – boardrider Apr 17 '17 at 7:39
  • \$\begingroup\$ Note that the different printing stanzas are quite similar: you may want to create a function for printing, and call it with the appropriate arguments. \$\endgroup\$ – boardrider Apr 17 '17 at 7:41
  • \$\begingroup\$ Your mistakesMade variable is actually not counting mistakes, but guesses: you may want to rename it. \$\endgroup\$ – boardrider Apr 17 '17 at 7:43
4
\$\begingroup\$

For the first function you could use set operations and the built-in string module:

import string

ALPHABET = set(string.ascii_lowercase)

def get_available_letters(letters_guessed):
    '''
    letters_guessed: iterable, what letters have been guessed so far
    returns: string, comprised of letters that represents what letters have not
      yet been guessed.
    '''
    return "".join(sorted(ALPHABET - set(letters_guessed)))

Note that I renamed your function/variables to adhere to Python's official style-guide, PEP8.

If you don't care that the return value is sorted, or a string, you can just return the set right away:

def get_available_letters(letters_guessed):
    '''
    letters_guessed: iterable, what letters have been guessed so far
    returns: set, letters that have not yet been guessed.
    '''
    return ALPHABET - set(letters_guessed)

In your actual game function, here are a few nitpicks:

  • Use str.format for an easier way to put some value somewhere in a text. This changes print ('I am thinking of a word that is ') + intro + (' letters long.') to print 'I am thinking of a word that is {} letters long.'.format(len(secretWord)) and print ('You have ') + str(mistakesMade) + (' guesses left.') to print ('You have {} guesses left.'.format(mistakesMade))
  • while mistakesMade > 0 and mistakesMade <= 8 and wordGuessed is False: could be while 0 < mistakesMade <= 8 and not wordGuessed
  • Use a set also for lettersGuessed, then if guess in lettersGuessed: becomes \$\mathcal{O}(1)\$, instead of \$\mathcal{O}(n)\$
  • if wordGuessed == True: can just be if wordGuessed:
| improve this answer | |
\$\endgroup\$
4
\$\begingroup\$

Since Graipher already did excellent job discussing get_available_letters() and more general structuring, I will look at the main loop. General discussion first, and then some restructured code below.

Name variables how they are used:

Your code uses a variable named mistakesMade, but its primary purpose is as guesses_left. So I renamed it.

Try not to test the same thing twice:

If you do not need to test a condition twice, then it is better not to. In the case of this code, I removed a couple of tests that were after the main loop. I did this by performing the action in the loop after the test, and then just breaking out of the loop. Some advantages in this case include that all of the logic in the while statement as well as the tracking variable were no longer needed.

Remove redundant code:

Anytime you find yourself writing the exact same code more than once, you should look for a restructure opportunity to remove the duplicate code. It is not always worth removing the first duplicate, but it often is. And by the time you get to three copies of the same code it is almost always good to restructure to get rid of the copies.

In this case prints that were exactly the same were a good clue that things could be simplified.

Loop Code:

while True:
    print('You have %d guesses left.' % guesses_left)
    print('Available letters: ' + getAvailableLetters(letters_guesses))
    guess = raw_input('Please guess a letter: ').lower()

    if guess in letters_guesses:
        msg = "Oops! You've already guessed that letter: "

    else:
        letters_guesses.append(guess)

        if secretWord == getGuessedWord(secretWord, letters_guesses):
            print('Congratulations, you won!')
            break

        if guess in secretWord:
            msg = 'Good guess: '
        else:
            guesses_left -= 1
            if guesses_left == 0:
                print('Sorry, you ran out of guesses. The word was '
                      + secretWord)
                break
            msg = 'Oops! That letter is not in my word: '

    print(msg + getGuessedWord(secretWord, letters_guesses))
    print('------------')
| improve this answer | |
\$\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.