3
\$\begingroup\$

This collects user's guesses for the word. The user has 8 guesses, a correct guess and a repeated guess does not affect the guess count.

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.
    '''
    import string
    fullstring = string.ascii_lowercase
    lettersLeft = ''
    for letter in fullstring:
        if letter not in lettersGuessed:
            lettersLeft = lettersLeft + letter
    return lettersLeft

def getGuessedWord(secretWord, lettersGuessed):
    '''
    secretWord: string, the word the user is guessing
    lettersGuessed: list, what letters have been guessed so far
    returns: string, comprised of letters and underscores that represents
      what letters in secretWord have been guessed so far.
    '''
    wordGuessed = ''
    for letter in secretWord:
        if letter in lettersGuessed:
            wordGuessed = wordGuessed + letter
        else:
            wordGuessed = wordGuessed + '_ '
    return wordGuessed

def isWordGuessed(secretWord, lettersGuessed):
    '''
    secretWord: string, the word the user is guessing
    lettersGuessed: list, what letters have been guessed so far
    returns: boolean, True if all the letters of secretWord are in lettersGuessed;
      False otherwise
    '''
    numCorrect = 0
    for letter in secretWord:
        if letter in lettersGuessed:
            numCorrect += 1
        else:
            return False
    return True

def hangman(secretWord):
    guessesLeft = 8
    lettersGuessed =[]

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

    while guessesLeft > 0:
        if isWordGuessed(secretWord, lettersGuessed):
            return print('Congratulations, you won!')
        print('You have ' + str(guessesLeft) + ' guesses left.')
        print('Available Letters: ' + getAvailableLetters(lettersGuessed))
        user_input = input('Please guess a letter: ')
        user_input = str(user_input)
        user_input = user_input.lower()

        if user_input not in lettersGuessed:
            lettersGuessed.append(user_input)

            if user_input in secretWord:
                print("Good guess: " + getGuessedWord(secretWord, lettersGuessed))
                print('-----------')
            else:
                print("Oops! That letter is not in my word: " + getGuessedWord(secretWord, lettersGuessed))
                print('-----------')
                guessesLeft -= 1
        else:
            print("Oops! You've already guessed that letter: " + getGuessedWord(secretWord, lettersGuessed))
            print('-----------')

    return print("Sorry, you ran out of guesses. The word was " + str(secretWord))

hangman('stackoverflow')
\$\endgroup\$
  • \$\begingroup\$ Your code doesn't run as-is. How are you intending to execute this? python hangman.py? \$\endgroup\$ – Thomas Ward Oct 7 '16 at 17:27
  • \$\begingroup\$ you just call the function on a word \$\endgroup\$ – ChrisIkeokwu Oct 7 '16 at 17:28
5
\$\begingroup\$

Multiple PEP8 Style Violations

There's a bunch of style violations here, and it really irks me. You are also violating fundamental parts of Python syntax with some of these things, especially with indentation, so you should try and correct style issues as well as my other concerns/suggestions. I try not harping on style, but there's enough issues here that it irks me enough to put this first.

  • import statements must be at the beginning of the file.
  • Function definitions need to be padded by two line breaks. That is, from the end fo one function to the start of the next, you need two blank lines between them (except inside a Class definition, which you don't have here).
  • Function names should be all lowercase, and multiple-word names for functions should be lowercase and separated with underscores (getGuessedWord should be get_guessed_word, etc.)
  • Variable names should be all lowercase, not CamelCase. (The exception are module-wide constants, which should be in all CAPS.). Multi-word variable names (such as lettersGuessed) should be lowercase and separated with underscores (letters_guessed)
  • Docstrings should be triple-quoted, not triple-apostrophe'd. (""", not ''')
  • You have indentation problems that make your code error out in certain cases under hangman where you will have unexpected indentation. (note that I suppress these when testing code here for review, but it's a critical issue). Use 100% consistent indentation - this should be 4 spaces per indentation level, not tabs, and you should not have some lines indented where they are not supposed to be.

User input can be condensed to one line

You currently use this for getting user input:

user_input = input('Please guess a letter: ')
user_input = str(user_input)
user_input = user_input.lower()

This can be condensed into one line, and we can also trim off the line break at the end too (so later recommendations for validation work without much issue):

user_input = input('Please guess a letter: ').lower().strip('\r\n')

No checking for valid inputs

You are expecting only letters to be provided, correct? That's the basis of Hangman.

We should probably test to see if we're actually getting letters provided to us, and make a warning. So, let's do this right after the user enters input:

user_input = input('Please guess a letter: ')).lower().strip('\r\n')
while user_input not in string.ascii_lowercase:
    print("You have not provided a letter, please only use letters!")
    user_input = input('Please guess a letter: ').lower().strip('\r\n')

Output messages: Code repetition

You've got this in here three times:

print('-----------')

You can instead use only one of these, and place it after the if/else block, and thereby not have to repeat this three times in a row inside the if block (note that I've snipped out all other contents of the if/else block, to shorten this post slightly).

if user_input not in letters_guessed:
    ....
else:
    ....

print('-----------')

Use augmented assignment statements.

This is currently in your code in getGuessedWord:

if letter in letters_guessed:
    word_guessed = word_guessed + letter
else:
    word_guessed = word_guessed + '_ '

Two things here: both of these can be condensed using augmented assignment statements, and you may wish to just use underscores without extra spaces between characters in the word, because it looks messy when I have some letters guessed and others not guessed (inconsistent spacing in output):

if letter in letters_guessed:
    word_guessed += letter
else:
    word_guessed += '_'

Use if __name__ == "__main__": to define what to execute when script is called from command line

python3 hangman.py will call hangman("stackoverflow"), but this is not really a good way to achieve it.

We should primarily define something like this at the end of the program to make sure that what runs is what we want to run when this is executed at the command line:

if __name__ == "__main__":
    hangman('stackoverflow')

Note though that you are always going to have "stackoverflow" as the string; you should strongly consider providing a file with valid words for usage, and then random selection therein rather than using a static string as an assignment.

This has an added benefit of 'guarding' the call to hangman() (which is the replacement for a call to main() as some would have) if, for some reason, I was importing parts of the module and importing into another one. This is more or less, also, a personal preference to guard whether I plan on importing or not, it's something I do regardless, just to protect calls to 'start' the program/script.


Unnecessary creation of variables

In get_available_letters, you do this:

fullstring = string.ascii_lowercase
....
for letter in fullstring:

Why do we create fullstring here? It's not needed - we can just use this instead and not have to create the fullstring variable:

for letter in string.ascii_lowercase:
\$\endgroup\$
  • \$\begingroup\$ This is my suggestions after my first look at your code - this may expand a little more as I keep looking at it \$\endgroup\$ – Thomas Ward Oct 7 '16 at 18:10
  • \$\begingroup\$ thank you very much!! But while I understand that the " if name == ... " portion of the code checks if you're in main or importing the code from a module. I don't understand why it's necessary or an improvement. Could you explain, please? \$\endgroup\$ – ChrisIkeokwu Oct 11 '16 at 20:07
  • \$\begingroup\$ @ChrisIkeokwu Mainly as a guard to the call of hangman(), if for instance I were to be calling hangman.py from something called game.py or similar, or importing parts here into other modules/scripts. Ultimately not needed if you only plan on running hangman.py, but my opinion is it's better to 'guard' that call than to not guard the call. \$\endgroup\$ – Thomas Ward Oct 11 '16 at 20:21
5
\$\begingroup\$

Lurking today. I just post this for your pleasure. It might give you some ideas about condensing your code.


Comments

In python there is something called annotations, they are very use full. The let you define for yourself and other readers of the code what in parameters and return parameters function has.

Example

def getGuessedWord(secretWord, lettersGuessed):
    '''
    secretWord: string, the word the user is guessing
    lettersGuessed: list, what letters have been guessed so far
    returns: string, comprised of letters and underscores that represents
      what letters in secretWord have been guessed so far.
    '''

compared to

def getGuessedWord(secretWord: str, lettersGuessed: list) -> str:
    ...

without the much expressive comment is that you wast a lot of space and make your code more hard to read. Of curse you should comment your code, but you don't have to comment on what the names of the function, secretWord, lettersGussed already states. But we don't know what type the parameters is from the names, but annotations can do that.

Comments should contain information on not told by the naming or annotation. Don't repeat yourself applies to comments as well.


Operators

This function

def isWordGuessed(secretWord, lettersGuessed):
    '''
    secretWord: string, the word the user is guessing
    lettersGuessed: list, what letters have been guessed so far
    returns: boolean, True if all the letters of secretWord are in 

    lettersGuessed;
          False otherwise
        '''
        numCorrect = 0
        for letter in secretWord:
            if letter in lettersGuessed:
                numCorrect += 1
            else:
                return False
        return True

is equivalent to the all operator like this:

all(l in guesses_letters for l in secret)

If you put a statement like this as is in a function, you are repeating your self. In the same sense that

def is_even(number):
   return number % 2 == 0:

is repeating what the statement already states.


Whenever you are dealing with user input which needs have to follow certain rules, it will always makes sense to separate it from other parts of the code like putting it in a function.

So by my interpretation of your code in which I started with your code, with out being as nice as you are, would be this.

from string import ascii_lowercase


def get_user_guess(guesses_letters: set) -> str:
    user_input = ""
    while user_input not in ascii_lowercase or len(user_input)-1:
        user_input = input("Guess Letter -> ")
        if user_input in guesses_letters:
            user_input = ""
    return user_input


def show(secret: str, guessed_letters: set):
    for l in secret:
        print(" {} ".format(l if l in guessed_letters else "_"), end="")
    print()


def hangman(secret: str):
    guesses = 8
    guesses_letters = set()

    while guesses and not all(l in guesses_letters for l in secret):
        c = get_user_guess(guesses_letters)
        guesses_letters.add(c)
        guesses -= 0 if c in secret else 1
        print("Guesses left {}".format(guesses))
        show(secret, guesses_letters)


def main():
    secret_word = "stackoverflow"
    hangman(secret_word)


if __name__ == '__main__':
    main()

Now why am I wining about don't repeat yourself? It is with this in mind:

"Every piece of knowledge must have a single, unambiguous, authoritative representation within a system"

this principal when coding will make your life easier, and readers life easier as well.

\$\endgroup\$
  • \$\begingroup\$ This is not really an answer, please comment on which points your code improves on the posted one. \$\endgroup\$ – Caridorc Oct 11 '16 at 19:37
  • \$\begingroup\$ Yes it was an answer. But I agree that I could have been explicit. \$\endgroup\$ – Simon Oct 12 '16 at 15:12
  • \$\begingroup\$ @Simon It looks real good now. Sure upvote from me for all the info \$\endgroup\$ – Caridorc Oct 12 '16 at 16:39
2
\$\begingroup\$

It is very good that you separated your logic from your user interface so it can more easily be improved.

Adding tests makes simplifying even easier and they also double as documentation so adding tests was the first thing I did.

Afterwards I proceeded re-implementing your functions by making use of the numerous python built-ins and comprehensions to reflect the meaning on a more abstract level avoiding re-inventing the wheel.

getAvailableLetters

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.

    >>> getAvailableLetters("acz")
    'bdefghijklmnopqrstuvwxy'
    '''
    return ''.join(sorted(set(string.ascii_lowercase) - set(lettersGuessed)))

Here I used the set built-in that mimics a mathematical set. Subtraction between two sets A and B means all the element of A not in B. I then use sorted to restore order as sets are un-ordered and join the result back into a string.

getGuessedWord

def getGuessedWord(secretWord, lettersGuessed):
    '''
    secretWord: string, the word the user is guessing
    lettersGuessed: list, what letters have been guessed so far
    returns: string, comprised of letters and underscores that represents
      what letters in secretWord have been guessed so far.

    >>> getGuessedWord("cattle", "at")
    '_ a t t _ _ '
    '''
    return ' '.join(char if char in lettersGuessed else '_' for char in secretWord) + ' '

I decided to also fix the un-even spacing as mentioned in another review. This is a use of a ternary conditional expression inside a generator comprehension. It is very simple and short.

isWordGuessed

def isWordGuessed(secretWord, lettersGuessed):
    '''
    secretWord: string, the word the user is guessing
    lettersGuessed: list, what letters have been guessed so far
    returns: boolean, True if all the letters of secretWord are in lettersGuessed;
      False otherwise

    >>> isWordGuessed("foobar", "broaf")
    True
    >>> isWordGuessed("foobar", "x")
    False
    '''
    return all(letter in secretWord for letter in lettersGuessed)

How would you express this in English? A secretWord is guessed if all of its letters are contained in the lettersGuessed. The code resembles really closely this "high-level" description and is thus very readable.

\$\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.