Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is a follow-up: Python Hangman Program

I'm working on adding a few features to my hangman game, and I just implemented two-player gameplay, in the form of one player chooses the word to be guessed while the other player guesses said word.

I got it to work in my program, but I feel like how I did it is a bit messy since it has largely the same code between two if statements but I'm not sure how to avoid this. You'll see it in my main function, down near the bottom of the program.

Any other thoughts on the program are appreciated.

#!/usr/bin/env python
# coding=utf-8
# <Omitted GPLv3 for code review>

import sys, random, os

if sys.version_info.major < 3:
    # Compatibility shim
    input = raw_input


class Gallows(object):
    def __init__(self):
        '''Visual of the game.'''
        self.wrongGuesses = 0
        self.image = ''
        self.states = [
            [
                '\t  _______ ',
                '\t  |     | ',
                '\t        | ',
                '\t        | ',
                '\t        | ',
                '\t        | ',
                '\t________|_',
            ],
            [
                '\t  _______ ',
                '\t  |     | ',
                '\t  O     | ',
                '\t        | ',
                '\t        | ',
                '\t        | ',
                '\t________|_',
            ],
            [
                '\t  _______ ',
                '\t  |     | ',
                '\t  O     | ',
                '\t  |     | ',
                '\t  |     | ',
                '\t        | ',
                '\t________|_',
            ],
            [
                '\t  _______ ',
                '\t  |     | ',
                '\t  O     | ',
                '\t \|     | ',
                '\t  |     | ',
                '\t        | ',
                '\t________|_',
            ],
            [
                '\t  _______ ',
                '\t  |     | ',
                '\t  O     | ',
                '\t \|/    | ',
                '\t  |     | ',
                '\t        | ',
                '\t________|_',
            ],
            [
                '\t  _______ ',
                '\t  |     | ',
                '\t  O     | ',
                '\t \|/    | ',
                '\t  |     | ',
                '\t /      | ',
                '\t________|_',
            ],
            [
                '\t  _______ ',
                '\t  |     | ',
                '\t  O     | ',
                '\t \|/    | ',
                '\t  |     | ',
                '\t / \    | ',
                '\t________|_',
            ]
        ]

    def get_image(self):
        '''Sets the current visual being used.'''
        self.image = '\n'.join(self.states[self.wrongGuesses])
        return self.image

    def increment_count(self):
        try:
            self.wrongGuesses += 1
            self.states[self.wrongGuesses]
        except IndexError:
            return False


class Wordlist(object):
    def __init__(self, wordfile):
        '''Set the length of the wordlist'''
        self.wordlist = wordfile
        self.numLines = 0
        # Get number of lines without placing entire wordlist in memory
        with open(self.wordlist) as file:
            for line in file:
                self.numLines += 1

    def new_word(self):
        '''Choose a new word to be guessed'''
        stopNum = random.randint(0, self.numLines-1)
        # extract word from file
        with open(self.wordlist) as file:
            for x, line in enumerate(file):
                if x == stopNum:
                    return line.lower().strip()


def create_blanks(word):
    '''Create blanks for each letter in the word.'''
    blanks = []

    for letter in word:
        # Don't hide hyphens
        if letter == '-':
            blanks += '-'
        else:
            blanks += '_'
    return blanks


def check_letter(word, guess, blanks, used):
    missed = False

    if guess.isalpha() == False:
        input("You have to guess a letter, silly!")
    elif len(list(guess)) > 1:
        input("You can't guess more than one letter at a time, silly!")
    elif guess in used:
        input("You already tried that letter, silly!")
    elif guess in word:
        for index, char in enumerate(word):
            if char == guess:
                blanks[index] = guess
        used += guess
    else: # If guess is wrong
        used += guess
        missed = True
    return blanks, used, missed


def endgame(won, word):
    print ('')
    if won:
        print("Congratulations, you win!")
        print("You correctly guessed the word '%s'!" % word)
    else:
        print("Nice try! Your word was '%s'." % word)
    return won


def play_again():
    while True:
        play_again = input("Play again? [y/n] ")
        if 'y' in play_again.lower():
            return True
        elif 'n' in play_again.lower():
            return False
        else:
            print("Huh?")


def game(word):
    '''Play one game of Hangman for the given word.
    Returns True if the player wins, False if the player loses.'''
    gallows = Gallows()
    blanks = create_blanks(word)
    used = []

    while 1 == 1:
        new_page()
        print(gallows.get_image())
        print(' '.join(blanks))
        print(' '.join(used))

        guess = input("Guess a letter: ")
        blanks, used, missed = check_letter(word, guess, blanks, used)

        if blanks == list(word):
            return endgame(True, word)
        elif missed:
            if gallows.increment_count() == False:
                return endgame(False, word)


def new_page():
    '''Clears the window.'''
    os.system('cls' if os.name == 'nt' else 'clear')


class _Getch(object):
    """Gets a single character from standard input.  Does not echo to the screen."""
    def __init__(self):
        try:
            self.impl = _GetchWindows()
        except ImportError:
            self.impl = _GetchUnix()

    def __call__(self): return self.impl()

class _GetchUnix(object):
    def __init__(self):
        import tty, sys

    def __call__(self):
        import sys, tty, termios
        fd = sys.stdin.fileno()
        old_settings = termios.tcgetattr(fd)
        try:
            tty.setraw(sys.stdin.fileno())
            ch = sys.stdin.read(1)
        finally:
            termios.tcsetattr(fd, termios.TCSADRAIN, old_settings)
        return ch

class _GetchWindows(object):
    def __init__(self):
        import msvcrt

    def __call__(self):
        import msvcrt
        return msvcrt.getch()



menu = '''
 ───────────────────────────────────────────────────────────────── 
| PyHangman                                                       |
|─────────────────────────────────────────────────────────────────|
| Press "q" to quit.                                              |
| Press "n" to to start a new game.                               |
| Press "h" for help with playing Hangman.                        |
| Press "i" to display info about this program.                   |
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -|
| In-game controls:                                               |
| <Enter> - Input a letter                                        |
 ───────────────────────────────────────────────────────────────── '''

player_menu = '''
 ───────────────────────────────────────────────────────────────── 
| How many players?                                               |
|─────────────────────────────────────────────────────────────────|
| Press "1" for 1 player.                                         |
| Press "2" for 2 player.                                         |
 ───────────────────────────────────────────────────────────────── '''

program_info = '''
 ───────────────────────────────────────────────────────────────── 
| PyHangman (ver. 1.4)                                            |
| (C) 2014 Alden Davidson <davidsonalden@gmail.com>               |
|─────────────────────────────────────────────────────────────────|
| PyHangman is a hangman game I wrote in python to practice my    |
| python skills as I began learning the language. This code is    |
| licensed under the GPLv3, so feel free to use it, share it,     |
| study it, and modify it as long as you adhere to the GPLv3; See |
| the source code for more information. Send all questions to the |
| email listed above.                                             |
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -|
| Press <Enter> to return to the Main Menu                        |
 ───────────────────────────────────────────────────────────────── 
 '''

game_info = '''
 ───────────────────────────────────────────────────────────────── 
| Hangman Rules and Instructions                                  |
|─────────────────────────────────────────────────────────────────|
| Objective: Guess the word before the man is hanged.             |
| Gameplay:  Guess letters, one at a time, to guess the hidden    |
|            word. Every time you guess incorrectly, another body |
|            part will be added, until the whole man is in the    |
|            gallows; if you guess incorrectly again you will     |
|            lose the game.                                       |
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -|
| Press <Enter> to return to the Main Menu                        |
 ───────────────────────────────────────────────────────────────── 
 '''


def main(wordfile='wordlist.txt'):
    wordlist = Wordlist(wordfile)
    getch = _Getch()

    while True:
        new_page()
        print(menu)
        user_input = getch()
        if user_input.lower() == 'q':
            new_page()
            print('Goodbye!')
            break
        elif user_input.lower() == 'n':
            new_page()
            print(player_menu)
            players = getch()
            if players == '1':
                while True:
                    game(wordlist.new_word())
                    if not play_again():
                        break
            elif players == '2':
                while True:
                    new_page()
                    word = input("Player 1, enter the word to be guessed:\n")
                    game(word)
                    if not play_again():
                        break
        elif user_input.lower() == 'h':
            new_page()
            input(game_info)
        elif user_input.lower() == 'i':
            new_page()
            input(program_info)

if __name__ == '__main__':
    try:
        main(sys.argv[1])
    except IndexError:
        main()
share|improve this question

3 Answers 3

One thing I would definitely change is the return type on functions like increment_count. You don't want to leave a situation where the returns aren't logical... ie) in most cases, all possible return values of a function should return a consistent amount of variables, and have consistent types.

def increment_count(self):
    try:
        self.wrongGuesses += 1
        self.states[self.wrongGuesses]
    except IndexError:
        return False

If this function hits an IndexError, it returns False otherwise it returns None by default. This makes code confusing and hard to maintain, seeing as most users would expect all cases to return booleans.

For example, its not standard to write lines like if gallows.increment_count() == False:, the proper style for evaluating a false boolean is:

if not gallows.increment_count():

However, in your code this will make the condition always evaluate to true, considering None evaluates as false,. ie)

>>> print not None and not False
True

So I would change the original function to something like bellow, such that you can evaluate the condition like above without worry of errors/bugs.

def increment_count(self):
    try:
        self.wrongGuesses += 1
        self.states[self.wrongGuesses]
    except IndexError:
        return False
    return True

...
share|improve this answer

@Calpratt spot the most important issue I think. I can only add nitpicks.


Instead of:

if guess.isalpha() == False:

should be:

if not guess.isalpha():

It seems you can delete _GetchUnix.__init__ because you're importing tty, sys in __call__ anyway.


The pep8 tool finds some Python style violations. You might want to fix those. (The docs of PEP8.)

share|improve this answer
1  
I'll definitely have to take a look at that, should help me cover some of the simple issues I have with my program to begin with! –  DaimyoKirby Apr 15 at 17:22

First of all, as pointed out by the other answers, you should try to follow PEP 8, the usually accepted style guide for Python. You can find tools to check your code automatically. You can find here the results for your code. Some points look like tiny details, some are more important, good thing is that all of them are easy to fix so you might as well do it :-)

Now, let's have a look at other things you could improve :

The Gallows class

Here is a nice little class, it has a constructor it keeps a state, update it and return meaningful information to anyone wanting it. Also, its name was a good chance for me to improve my vocabulary. However, I am not quite sure it deserves to be a class.

First thing,let's have a look at the three attributes :

  • self.states is a list of ASCII art drawings that won't be updated. Also, its content will be the same for all instances of Gallows. We don't need to have this repeated for all instances, do we ? We could make this a constant and move it out of the scope of the class.

  • self.wrongGuesses is the number of wrong guesses so far. It looks pretty good.

  • self.image is a piece of drawing retrieved from self.states. Actually, it is the piece of drawing at index self.wrongGuesses. Because we have no good reason (as in performance), we might want to get rid of a piece of duplicated information : the number of wrong guesses in itself should be enough.

At the end, we have a class with a single integer attribute with no really smart logic around it (no offense), the whole thing could become :

GALLOWS_ASCII_ARTS = ['...', ..., '...']
def get_gallows_ascii_art(number_of_guesses):
    '''Get the current visual to use.'''
    return '\n'.join(GALLOWS_ASCII_ARTS[number_of_guesses])

Please note that I have taken the freedom to remove the try/catch. It something goes wrong, we'll notice more quickly and its close to the "keep it simple principle".

Now, I am not entirely done with this. Indeed, the join on the elements of GALLOWS_ASCII_ARTS could be done once and for all by doing it as you build the list.

If you have found this comment interesting, you might like this talk.

The def create_blanks(word) function`

You can rewrite your function with list comprehension :

def create_blanks(word):
    '''Create blanks for each letter in the word.'''
    blanks = []

    for letter in word:
        # Don't hide hyphens
        if letter == '-':
            blanks += '-'
        else:
            blanks += '_'
    return blanks

becomes

def create_blanks(word):
    '''Create blanks for each letter in the word.'''
    blanks = []
    for letter in word:
        # Don't hide hyphens
        blanks+= ('-' if letter == '-' else '_')
    return blanks

which becomes :

def create_blanks(word):
    '''Create blanks for each letter in the word.'''
    blanks = ['-' if letter == '-' else '_' for letter in word]

The def check_letter(word, guess, blanks, used) function

This function gets an unguessed letter from the user and updates blanks. To keep things maintainable, it should do only one thing. Also, it could be a good idea to make the guess = input("Guess a letter: ") part of the function checking the input.

For instance something like :

def get_valid_guess_from_user(used):
    while True:
        guess = input("Guess a letter: ")
        if not guess.isalpha():
            input("You have to guess a letter, silly!")
        elif len(guess) != 1:
            input("You can't guess more than one letter at a time, silly!")
        elif guess in used:
             input("You already tried that letter, silly!")
        return guess

Please note that I took this chance to remove a useless call to list and to remove the comparison to False.

The def game(word) function

Because I got rid of the gallows class here, we have to keep track of the number of wrong guesses. Different strategies here :

  • because we want to track already used letters, we might just rely on the numbr of used letters : if you do this, you have not to consider "correct" letters as used.

  • have a simple counter you increment.

In any case, instead of storing used letters in a list, you should probably use a set because the order does not matter and letters will not appear more than once.

share|improve this answer

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.