Python 2.7 Tower of Hanoi game with ASCII graphics

Question

The game generates a fixed, "animated" game board via ASCII characters that is reprinted and cleared after each valid move. The format of the input for making moves is described in the script. You select the "difficulty" via a commandline parameter, so the game requires you enter a number (>=3) for the disc count after the filename.

I'm new to classes, so I wasn't sure how to retrieve the latest "top occupied position" on a rod following each addition/removal of a piece besides what I have here and maybe one other way. I'm guessing that I could have had a default self.topindex and self.top for a rod, and then updated this upon each call of the methods add() and remove().

import os
import sys
level = int(sys.argv[1])

class rod:
    def __init__(self,n):
        self.n=n
        if n==1: self.state= list(range(1,level+1))
        else: self.state=[0]*level
    def pos(self, height): #This generates one chunk of the rod and any pieces at that height
        width=self.state[height-1]
        return " "*(level+1-width)+"-"*width+"|"+"-"*width+" "*(level+1-width)  
    def topindex(self): #This is meant to return the index of the top occupied position on the rod
        for index,i in enumerate(self.state):
            if i!=0: return index
            elif index==level-1: return index #But if there are no occupied positions (empty rod) it should return the bottom
    def top(self): 
        return self.state[self.topindex()]
    def add(self, piece): 
        if self.top()==0: self.state[level-1]=piece
        else: self.state[self.topindex()-1]=piece
    def remove(self):
        self.state[self.topindex()]=0

rod1=rod(1) #This seems like a waste of lines
rod2=rod(2)
rod3=rod(3)

gamestate=["Welcome to the game. ", "first"]

def printgame(): 
    print gamestate[0]+"The objective is to move all the pieces from rod 1 to another rod."
    print "Pieces must be placed in ascending order of size from top to bottom." 
    print 'Move pieces by typing "n m" (where n and m denote the numbers of the source and destination rods) when requested.'
    print ""
    for i in range(1,level+1):
        print rod1.pos(i)[1:]+rod2.pos(i)+rod3.pos(i)
    spacing=(level-3)*" " #Add each block of each rod at the same height to one line, then print the line
    print spacing+" Rod 1    "+2*spacing+"Rod 2    "+2*spacing+"Rod 3\n"

def hasdigit(string):
    return any(char.isdigit() for char in string)

os.system("clear")

def movepiece():
    global rod1 #Too many globals here
    global rod2
    global rod3
    global gamestate
    move=raw_input("Which piece will you move {}? ".format(gamestate[1]))
    if " " in move and hasdigit(move.split(" ")[0]) and hasdigit(move.split(" ")[1]):
        gamestate[0]=""
        gamestate[1]="next"
        source=move.split(" ")[0]
        destination=int(move.split(" ")[1])
        piece=eval("rod"+str(source)+".top()")
        if (piece<eval("rod"+str(destination)+".top()") or eval("rod"+str(destination)+".top()")==0) and piece!=0:
            eval("rod"+str(source)+".remove()")
            eval("rod"+str(destination)+".add("+str(piece)+")")
        elif piece==0:
            print "There's nothing to move from rod {}!\n".format(source)
            movepiece()
        else:
            print "You can't move that there!\n"
            movepiece()
    elif move=="exit": gamestate=move
    else: 
        print "Please use the specified format: n m (where n and m denote the numbers of the source and destination rods).\n"
        movepiece()

steps = 0
while rod2.state!=list(range(1,level+1)) and rod3.state!=list(range(1,level+1)):
    if gamestate=="exit": break
    printgame()
    movepiece()
    steps+=1
    os.system("clear")

if gamestate!="exit":
    printgame()
    print "Congratulations, you've finished the game in {} steps.".format(steps)

Answer 1

Hi thanks for sharing your code,

You should stick to Python naming conventions, check out the PEP8 style guide.

https://www.python.org/dev/peps/pep-0008/

Based off of this, your rod class should be class Rod: and movepiece should be move_piece

For your global variables, you could fix this by having a Game or Hanoi class.

class Hanoi:

    def __init__(self, rod1, rod2, rod3):
        self.rod1 = rod1
        self.rod2 = rod2
        self.rod3 = rod3
        ...

Watch out for using eval. There are lots of reasons not to use eval, but the one I want to talk about is that here you're relying on the user to know variable names. Variable names should never matter. But here if you were to change a variable name, your program would not function as intended.

If the goal is to call a method on a Rod object based on user input, just do a standard if/elif block

if source == 1:
    piece = self.rod1.top()
elif some_condition:
    ...

If you're using eval to call methods like this, just call the methods instead.

list(range(1,level+1)) if you look at the documentation for the range function, it already returns a list, you can just use range(1,level+1)

There seem to be some unnecessary offsets throughout your code, why use range(1, level + 1) and not just range(level) you can just take the user input value and subtract 1 from it in one place, in Python counting starts from 0 :)

def __init__(self,n): n is a very poor name for a variable, use something like num or number instead, anything to improve readability goes a long way.

in your movepiece() function, you call movepiece again inside, I don't think there's any real benefit to using recursion here, I think it would be simpler to understand a simple loop instead, and it would also be simpler to reason about and leave less room for error.

while !game_over:
    # game loop

This way you don't need to worry about maximum recursion depth exceeded either, not that that would really be a problem here anyway.

This while loop rod2.state!=list(range(1,level+1)) and rod3.state!=list(range(1,level+1) could be a lot simpler.

Try breaking it down into multiple well named boolean values instead.

meaningful_name_for_this = rod2.state!=list(range(1,level+1))
meaningful_name_for_that = rod3.state!=list(range(1,level+1)
while meaningful_name_for_this and meaningful_name_for_that:
    # do your loop code

Make your code as self documenting as possible.

destination=int(move.split(" ")[1])

Avoid magic numbers where possible, whenever you need specific values like this, make them a well named constant instead.

destination=int(move.split(" ")[DESTINATION_INDEX])

especially if you reuse them throughout your code base.

Hopefully some of this was useful to you!

Edit:

Here's an example of what your move_piece method could look like Here it's a method of some Hanoi or Game object.

def move_piece(self):
    move = INVALID_VALUE  # some constant
    while not self._is_move_valid(move):  # method that checks if a move is okay or not
        move = self._prompt_for_move() # reads user input

    # if we're here, we know that the move is definitely valid
    self._perform_movement(move)  # does the work on the Rod objects

I've made up some methods here, in your code you have the logic to do these actions, so I'll let you fill in those!

Notice now how short this method is, the shorter the method the better, even though it's made up of many calls to other methods, by giving them all good clear names, you should be able to see what the method is doing without looking at the bodies of these other methods.