[SOLVED] Python Hangman Game

DaimyoKirby · 2014-02-17 20:49:04

Hello fellow Archers, budding programmer here!

I'm currently working my way through Learn Python the Hard Way, and to exercise my python skills I wrote a little hangman script. All it does is get a random word from a wordlist, draw a gallows, and lets you play hangman.

If you would like to simply view the source code before/without downloading anything, I've posted it on Pastebin.
You can find it here: PyHangman.py [direct download]
You can use any dictionary for your wordlist, as long as it is called 'wordlist.txt' or you change the source code appropriately. I use the 12of12inf Dictionary, part of the 12Dict package found on Kevin's Word List Page.

So why am I posting it here?
Well, I'm just curious as to what people think of it, especially concerning my coding. To me it feels like a bit of a spaghetti code, and that I overused (abused?) OOP style in it. For example, I ended up passing most of my objects through every function - it's hard to explain, but if you look at the code you'll probably quickly see what I mean.

So a few questions:
How can my code be improved structurally? It's fine if you post straight-up corrections to my code, but I'd prefer if you just drop hints to nudge me in the right direction, since I want to try to learn this stuff, not just copy it.
Am I using OOP style correctly?
Any other thoughts/comments/constructive criticisms about my program in general?

Edit: a few grammer/spelling edits, added short description of the game (woops!), added link to pastebin for easy viewing

Last edited by DaimyoKirby (2014-08-24 15:00:50)

mychris · 2014-02-17 22:11:55

Since coding style is a matter of taste (I just like my own style ) it's very hard to give good hints, so just my 2 cents:

First of all, always use a python version in your hashbang! Your code is for python2, but my standard python is python3, so it won't run on my machine without touching the code.

Your style is very clean. It's easy to follow (just looked at it for a minute and everything was clear) so, well done. Maybe you should look up the standard library, because for some problems there is a cleaner and nicer way than yours:

self.numLines = sum(1 for line in open('wordlist.txt'))

self.numLines = len(open('wordlist.txt').readlines())

def set_state(self, misses):
    '''Sets the current visual being used.'''
    newState = ''
      
    state = self.state[misses] # set state to the list of desired gallows image
    # construct gallows image into str from list
    for piece in state:
        newState += piece + '\n'
    return newState

def set_state(self, misses):
    '''Sets the current visual being used.'''
    return '\n'.join(self.state[misses])

But that's normal if you are learning a new language (and of course a matter of taste which one is "cleaner and nicer" ).

The initialization of your Engine is not very nice. The constructor (__init__) should initialize your object with all the information it needs. So there should be no setup function. And if there is a setup function it should never call __init__. Just initialize the Engine if you need it and with alle the informations it needs.

Another thing I want to say is, that you are using too many classes. I know "Separation of concerns", "Modularity" and all these things, but for me your design doesn't feel right. It's too bloated because of all these classes and objects and passing around these things. You can see that your design has some flaws because EndGame needs the same informations as your Engine, but it does nothing with these informations. It just calls setup on the Engine. Nothing more. So maybe the Engine should hold the setup and EndGame just call newGame (or something like that) on the Engine.
So yeah, your feeling is right. Maybe the Engine should only handle one game and be reinitialized for each game? Maybe the Engine should handle the whole game, but have another class (like Game or something) handle the game which is currently active? I don't know, it's hard to answer so try it your self.

But one can talk about design the whole day and just be wrong, so I won't say anymore.
Your code is clean, maybe not "pythonic" enough

import this

but you will need some time to learn a new language. Look around at some github projects using python (better not mine, those are just ugly and hacked together...) and read some code. The best way to learn a new language is to use it and to read code.

In addition, if you just started learing python, try Go. Since I started programming in Go, I quit python and will never come back. It's a nice language, but will never be as nice as Go .

ewaller · 2014-02-17 22:18:06

As was pointed out above, your code as written invokes the Python 3 on Arch. Your code requires Python 2.
Running your program as python2 PyHangman.py did not work as you had non ascii characters in line 4 without having specified a character encoding.

You do not handle upper and lower case well. I created a word list with both upper and lower case words. The answer is case sensitive. Were I you, I would force everything to lower (or upper) case before making comparisons.

Aside from that, from my black box testing (testing done without reading your code), it looks pretty good. Now, I will go read your code...

ewaller · 2014-02-17 22:56:01

I am of the opinion one should use only new style classes. tl;dr: I would make all of your classes sub classes of object.

Now, lets talk about:

class EndGame:
    def game_over(self, game, image, getWord, letters):
        newPage()
        print "Nice try! Your word was '%s'." % game.word
        while True:
            play_again = raw_input("Play again? [y/n]")
            if 'y' in play_again:
                game.setup(image, getWord, letters, self)
            elif 'n' in play_again:
                sys.exit(0)
            else:
                print "Huh?"
    
    def victory(self, game, image, getWord, letters):
        newPage()
        print "Congratulations, you win!"
        print "You correctly guessed the word '%s'!" % game.word
        while True:
            play_again = raw_input("Play again? [y/n]")
            if 'y' in play_again:
                game.setup(image, getWord, letters, self)
            elif 'n' in play_again:
                sys.exit(0)
            else:
                print "Huh?"

First, this does not really need to be a class. It could just be some function definition. That would prevent your having to pass finish around in your game play. Also, these two functions are very similar, they could (should) be combined. Also, you exit from inside an infinite loop -- that is okay. What is not okay is to start the next game from inside an infinite loop. I think that creates a memory leak. What you should do is return from the function and then start a new game from the engine.

consider rewriting this as

    def EndGame(result="lose"):
        newPage()
        if result != 'lose':
            print "Congratulations, you win!"
            print "You correctly guessed the word '%s'!" % game.word
        elseif:
            print "Nice try! Your word was '%s'." % game.word
        while True:
            play_again = raw_input("Play again? [y/n]")
            if 'y' in play_again:
                return
            elif 'n' in play_again:
                sys.exit(0)
            else:
                print "Huh?"

calling this is EndGame() or EndGame("lose") will hand that case, anything else is the win case.
Then in main, all you need to do is:
while True: game.setup(image, getWord, letters, finish)
And, fix the call to victory or endgame.

As to OOP. No, sorry. You are not really there yet. Your code is very procedural and happens to define classes. Give me a few minutes and check your personal email

DaimyoKirby · 2014-02-20 06:08:51

Ok, sorry about the slow response, schools been a bit busy for me as of late. I apologize in advance for the wall of text.

mychris - the reason I used 'sum' instead of 'len' with '.readlines' is because readlines apparently places the entire file into memory, which isn't a very good thing to do since the file could potentially be very large. I know that's not really the case with this tiny hangman program, but I thought "why not?". Anyway, I found this info in a StackOverflow answer.

I didn't know it was an explicitly bad thing to do, calling the constructor - it was the first way that came to mind to get the game to set itself up correctly when the user wanted to play again. That being said, it did feel kinda weird to being looping itself of a sort in that kinda weird backdoor way...but I'm definitely going to avoid that from now on. The Engine class is kind of a mess, and I'm definitely going to rethink how to implement that.

----------------------------------

ewaller -
I was writing it initially on my school mac, and didn't think about (nor know much about) setting the default environment. I've added it as

#! /usr/bin/python2

Is that correct? It seemed to work right when I ran the script with './PyHangman.py' on my Arch install (after chmod-ing it, of course).

As for the ascii error, silly mistake - it was failing to read the copyright symbol I had copied in. For now I've removed it, but is there a usually-suggested encoding (utf-8, latin-1, etc) to use? Or is it best to just leave it as ascii?
Cases - that's a really good point; I'll make sure to fix that.

The whole object thing confuses me a bit, but what I think I've understood is basically that using new style classes gives me access to some extra tools and unifies my classes/objects under a single type.

What you said about EndGame makes sense - the memory leak would be created since the program would create yet another while-loop inside the previous while-loop each time the user asked to play again?
In your corrected code example, of the endgame, when it returns, this would close that instance of the game, then open a new one since the setup function is in a while-loop?

Thanks for the email, ewaller, the examples are really helpful. I think I get the basic gist of OOP that classes are "templates" of sorts that can be used to create objects with a predefined set of functions, but past that it's still a bit over my head. And after looking at the shape example, I definitely need to keep reading up on how classes work together, and all the other topics you listed (virtual functions, inheritance, etc.), since that one took my brain for a bit of a whirl.

So after working through what you've both said, I've got a clearer image of where I should be taking my code. In the next couple days I'm going to finish implementing the changes you've both suggested (I've already hit most of them), and then I'm going to nuke most of my unneeded classes into functions and rewrite the inner workings of the script (Engine and main, I'm looking at you). I'll post the revised code (ver 1.1!) when that's all done.
Thanks for the great initial feedback, guys.

Arch Linux

#1 2014-02-17 20:49:04

[SOLVED] Python Hangman Game

#2 2014-02-17 22:11:55

Re: [SOLVED] Python Hangman Game

#3 2014-02-17 22:18:06

Re: [SOLVED] Python Hangman Game

#4 2014-02-17 22:56:01

Re: [SOLVED] Python Hangman Game

#5 2014-02-20 06:08:51

Re: [SOLVED] Python Hangman Game

Board footer