6
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ Is this complete? It sounds like you're still trying to add something. \$\endgroup\$ – Jamal Aug 19 '17 at 22:27
  • \$\begingroup\$ Well it works as a complete Tower of Hanoi game so no. There are always more options I could add on like a time limit but I'm asking about this version. \$\endgroup\$ – Stinky Peterson Aug 19 '17 at 22:36
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Hi, thanks for all the advice! I've mainly been learning through working on random examples so I've been putting off learning standard naming conventions, that's a good idea. Just to clarify, the reason I call movepiece() again in movepiece() is to avoid having it called through the while loop. If called through the while loop, this would add another "step" even though no legitimate move has been made. I can only think of adding a default legitimate=False to the start of the while loop and having this changed to legitimate=True only a legitimate move is made. \$\endgroup\$ – Stinky Peterson Aug 20 '17 at 8:49
  • \$\begingroup\$ Hi I added a short example at the end of what your move_piece method could look like! \$\endgroup\$ – chatton Aug 20 '17 at 9:31
  • \$\begingroup\$ "If the goal is to call a method on a Rod object based on user input, just do a standard if/elif block" Wouldn't this take up more space than using eval() though? What's the purpose? If I ended up changing the variable name for rod except in this block, both of the approaches would stop working anyway \$\endgroup\$ – Stinky Peterson Aug 20 '17 at 9:31
  • \$\begingroup\$ Thanks, it does make more sense putting the format-checking part directly in the move_piece() method \$\endgroup\$ – Stinky Peterson Aug 20 '17 at 9:33
  • \$\begingroup\$ using a few more lines won't hurt anyone! I think it will make your intent far more clear too for anybody reading your code. Yes you're right it would break either way, but say you had a larger project and you used eval throughout, and you needed to change some variable names, I'm not sure if any refactoring tools would be smart enough to update the eval statements too (they might!) but you'd have no problem refactoring a few variable names. but also, see this question stackoverflow.com/questions/1832940/… \$\endgroup\$ – chatton Aug 20 '17 at 9:35

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.