current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 6 down vote favorite

I'm new to programming (just about 2 months), and have been trying to learn as much as I can as quickly as I can.

I made a little Hangman game as a practice project this weekend. It doesn't have visuals — the user is allowed 5 strikes. I used the enable1.txt list of words.

Anyway, I was hoping to get some feedback on the game: specifically anything that can be improved or any major problems that I can notice now before they become ingrained bad habits.

import sys, time, random

# Initially sets 'word_list', 'word' (the secret word), and 
# 'revealed' (the correct letters revealed to user) to None.
# These values will be set anew each time game starts
word_list = None
word = None
revealed = None

# Initially sets the number of strikes, hints, and 
# letters already guessed to None. These values will be 
# updated during game play
strikes = 0
hints = 0
already_guessed = []

def giveHint():
    """
    Fills in one of the missing letters for the user.
    """
    global hints


    while True:

        if hints < 3:

            # Loops through letters in the secret word until it finds one
            # the user hasn't already guessed
            while True:

                letter = random.choice(word)

                if letter not in revealed:
                    hints += 1
                    print "\n      HINT: '%s'    " % letter
                    print "   Hints used: %d/3  " % hints
                    return letter
                    break
        else:
            return None 
            break

        break


def makeGuess():
    """
    Provides Strikes and Hints status.
    Then gets choice from user (a guess or a hint).
    """
    global strikes, hints

    # Provides game status to user
    print '_' * 40
    print
    print ">>>>>>> STRIKES: %d/5 <<<<<<<" % strikes
    print ">>>>>>>  HINTS:  %d/3 <<<<<<<" % hints

    # Prints letters user has already guessed;
    #  set() used to only give a particular letter once,
    #  list() used to put those unique letters in a list,
    #  sorted() provides them in alphabetical order.
    print "Already Guessed Letters: ", ' '.join(sorted(list(set(already_guessed)))) 
    print "\nTHE BOARD:"
    print ' '.join(revealed) 
    print
    print "Guess a letter or the word"
    print "To get a hint, enter '#hint':"

    guess = raw_input("> ").lower()

    # Creates blank space to distinguish turns in the game from eachother
    print "\n" * 5 
    print "_" * 40
    print

    # If user asks for a hint, assign result of giveHint() to guess variable
    if '#hint' in guess:
        guess = giveHint() 

    return guess



def revealWord(guess=None):
    """
    Prints out the secret word letter by letter
    """
    print "THE SECRET WORD IS:",

    for i in range(6):
        sys.stdout.write('. ',)
        sys.stdout.flush()
        time.sleep(.3)

    for letter in word:
        sys.stdout.write(letter,)
        sys.stdout.flush()
        time.sleep(.3)
    print
    return



def hangman():
    global strikes, hints, already_guessed, revealed, word

    # As long as there is a blank space in the "revealed" list,
    #  the user continues to guess letters, unless strikes > 5 
    #  or the user guesses the whole word
    while '_' in revealed and strikes < 5:

        guess = makeGuess()

        if strikes == 5:
            break

        # guess == none when giveHint() returns None
        # no break allows the while loop to continue 
        elif guess == None:
            print "SORRY %d/3 hints used" % hints 

        # If the user guesses the whole wordcorrectly, 
        # 'revealed' variable set to 'word', which allows 
        #  the winning outcome condition to be met.
        elif len(guess) > 1:
            print "You guessed '%s'\n" % (''.join(guess))
            if guess == word:
                revealed = word 
            break

        elif guess in already_guessed:
            print "You've already guessed that letter!"


        elif guess in word:

            print "Nice! '%s' is in the word" % guess

            for index, letter in enumerate(word):            
                if guess == letter:
                    revealed[index] = guess
                    already_guessed.append(guess)


        elif guess not in word and strikes < 5:
            print "Sorry '%s' not found!...." % guess

            strikes += 1
            already_guessed.append(guess)

        else:
            return
            break


    # Outcomes:
    # -------------------------------------------------------
    # Losing outcome if strikes >= 5        
    if strikes >= 5:
        print  "*" * 10, "    STRIKE 5!     ", "*" * 10
        print  "*" * 10, " SORRY, YOU LOSE. ", "*" * 10   
        revealWord()

        # resets strikes, hints, and already_guessed list
        strikes, hints, already_guessed = 0, 0, []

        # returns to starting menu
        start()
        return

    # Winning outcome if no empty slots left for guessing
    elif '_' not in revealed:
        revealWord()
        print "*" * 10, " CONGRATS, Ya Got It! ", "*" * 10

        # resets strikes, hints, and already_guessed list
        strikes, hints, already_guessed = 0, 0, []

        # returns to starting menu
        start()
        return

    # Losing outcome if the wrong word was guessed  
    else:
        print "Sorry....You Lose."
        revealWord(guess)

        # resets strikes, hints, and already_guessed list
        strikes, hints, already_guessed = 0, 0, []

        # returns to starting menu
        start()
        return




def start():
    global word_list, word, revealed

    # opens enable1.txt file and assign random word to variable 'word'
    word_list = open("enable1.txt", 'r').readlines()
    word = random.choice(word_list).replace('\n','').replace('\r','')

    # sets 'revealed' variable to '_'s the length of the chosen random word
    revealed = ['_' for i in range(len(word))]

    # Gives user choice to play or quit
    print
    choice = raw_input("Press 'Y' to Play\nOr 'Q' to Quit: ")
    if choice.lower() == "y":
        hangman()
    else:
        print "Goodbye!"
        sys.exit()





print
print "Welcome to HANGMAN."
print "-------------------"

start()
share|improve this question
up vote 2 down vote accepted

The Python style guide has a lot of good suggestions about code readability and good conventions. It's a highly worthwhile read. Some quick notes that would be relevant to you. Put imports on separate lines, not all on one. You use snake_case for variables, but use it for functions too. And one line docstrings should be on one line, ie make give_hint's one look like this:

def giveHint():
    """Fills in one of the missing letters for the user."""

I agree that it's good you're thinking a lot about including comments to make your code clear, but I think you're being overzealous.

# Initially sets 'word_list', 'word' (the secret word), and
# 'revealed' (the correct letters revealed to user) to None.
# These values will be set anew each time game starts

We can tell that they're set to None, word_list = None does a good job of that. And it's easier to know that the values are reset on each new game if you just set them at the start of each new game, which you actually do. I presume these are here in order to try work with global, but instead just define them in start and then pass them to functions as they're needed.

Also it's bad form to have contradictory comments. Your next comment talks about setting values to None but it actually sets them as 0 and []. Keep comments up to date, otherwise they'll only introduce confusion (especially in cases where it's not obvious that the comments don't match the code).

You seem to misunderstand a bit about flow in give_hint. You call while True, but then you immediately test an if condition. Just make that the expression with while. You then use while True to loop through all the letters of the word, but instead you could just iterate over the letters. Of course you don't want to loop over them in order, but you can quickly get a randomly shuffled list with random.shuffle.

Also you don't need to break after a return. return will always exit the function immediately, so no further lines will be executed. Also return will return None by default, so just putting return will prematurely end a function and return None. In fact, if no return is reached at all, then None is returned by default anyway.

Though, I would recommend actually informing the user that they're out of hints rather than silently returning None and I'd also return an empty string instead as that matches the type of the intended output.

Lastly, use str.format, as that's the new way to format strings. Instead of needing % signs, str.format will replace {} that are in the string. It converts things to strings and has other nice formatting options too so it's good to be used to.

Here's how I'd rewrite give_hint:

def give_hint(word, revealed, hints):
    """Fills in one of the missing letters for the user."""

    if hints == 3:
        print ("You're out of hints!")
        return ''
    while hints < 3:
        # Loops through letters in the secret word until it finds one
        # the user hasn't already guessed
        for letter in random.shuffle(word):
            if letter not in revealed:
                hints += 1
                print "\n      HINT: '{}'    ".format(letter)
                print "   Hints used: {}/3  ".format(hints)
                return letter

Note that I did also pass in word, revealed, hints because as I said, it's better not to use global.

You have one line where you turn already_guessed from a list into a set and then back to a list to sort it. Why not make it a set in the first place? You can't use append on a set, but you can use add. The only time you need already_guessed to be a list is when you're sorting it, in which case you need to convert it back and forth anyway.

You have guess=None in revealWord, but why? You've set guess as a default parameter, which just means that if no value is passed for guess then within revealWord guess will be set as None. You never use guess in that function anyway, and it wont affect the value outside that function so it's not really benefiting anything.'

I'd break up hangman a bit, maybe put the end game state in its own function, as you have a lot of stuff in that one function and the rest of your code is dividing up tasks nicely.

And instead of ending by calling start again, wrap your start function's code in a while loop:

while True:
    choice = raw_input("Press 'Y' to Play\nOr 'Q' to Quit: ")
    if choice.lower() == "y":
        hangman()
    else:
        print "Goodbye!"
        break

Note you can also just break instead of needing sys.exit().

share|improve this answer
    
This is awesome and much appreciated. Thanks a ton. I'm going through it slowly to make sure I understand everything. I'll also be taking a closer look at the Python style guide. Regarding 'guess=None' in revealWord, that was a mistake to be included -- I had originally designed it to pass a guess to to revealWord, but removed that and forgot to remove the parameter. – mose Oct 27 '15 at 23:07
    
@mose Ah, that makes. I thought you had just misunderstood the use. Glad to help! – SuperBiasedMan Oct 27 '15 at 23:15
up vote 2 down vote

Nice code, really enjoyed reading this. Well-documented and understandable.

Minor comments:

  • Use this construct to start program - this will make your code re-usable as you will be able to run this file by itself and also include into other module:
if __name__ == "__main__":
    start()
  • Avoid creating global variables - you can easily create a class to contain all variables required for you and also all the methods of the game. If you will be creating mult-module program - this will help to avoid global mames conflics and is generally cleaner.

Read about python modules to make your code re-usable.

share|improve this answer
    
Thanks a lot! I'd seen that name__==__main construction in code but hadn't figured out. I'm looking forward to starting to experiment with using multiple modules. Also thanks for the suggestion to use a class to contain all the variables. I'd been trying to figure out a way to get rid of needing to use the globals and hadn't figure it out. – mose Oct 26 '15 at 1:13

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.