Iyfjky

Question

I have just finished a small program for Tower of Hanoi using recursion. I did it to practice OOP which I recently learnt. It took me about 2 hours to research about the recursive algorithm and write out the code. It works, after much debugging... However, after comparing to other implementations, I realised my code is rather long, plus unconventional when it comes to how I approach and execute as well as the naming conventions.

Could I have some feedback about the program? Thank you!

class rod:
 move_count = 0

 def __init__(self,disks=,position=""):
 self.diskslist=disks
 self.rod_position=position
 # Remove the top disk of a rod
 def remove_top(self):
 print(f"Move the disk of size self.diskslist[0].size from self.rod_position Rod ", end="")
 rod.move_count += 1
 return self.diskslist.pop(0)
 # Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it
 def add_to_top(self,current_disk,announce=True):
 if len(self.diskslist) == 0:
 if announce==True:
 print(f"on to the top of the self.rod_position Rod", end="n")
 self.diskslist.insert(0,current_disk)
 return True
 else:
 if current_disk.size < self.diskslist[-1].size:
 if announce == True:
 print(f"on to the top of the self.rod_position Rod", end="n")
 self.diskslist.insert(0,current_disk)
 return True
 else:
 return False


class disk:
 num=0
 def __init__(self,size):
 self.size=size
 disk.num += 1


#Generating 8 disks of increasing size
disk_num=8
disks = 
for i in range(disk_num):
 disks.append(disk(i))

#Generating 3 rods
rods = 
rods.append(rod(,"Left"))
rods.append(rod(,"Middle"))
rods.append(rod(,"Right"))

# Attaching all disks to the left rod
for i in range(len(disks)):
 rods[0].add_to_top(disks[len(disks)-i-1],False)


def recursive_solve(current_rod, lower_range, target_rod):
 # If only moving the top disk, execute it
 if lower_range == 0:
 rods[target_rod].add_to_top(rods[current_rod].remove_top())
 # If not, keeping simplifying the recursion.
 else:
 alt_rod = 3 - current_rod - target_rod
 recursive_solve(current_rod, lower_range - 1, alt_rod)
 recursive_solve(current_rod, 0, target_rod)
 recursive_solve(alt_rod, lower_range - 1, target_rod)

print(f"The steps needed to move disk_num pieces of disks are:")
recursive_solve(0,len(disks)-1,2)

for i in range(len(rods)):
 print(f'n For rod number i+1:',end=" ")
 for j in range(len(rods[i].diskslist)):
 print(rods[i].diskslist[j].size,end=" ")

print(f'n The number of moves taken in total is rod.move_count')

score 2 · Answer 1 · 2018-10-15 13:16:42Z

Your code seems to be working and trying to use OOP is a nice touch.
Let's see what can be improved.

Style

There is an official standard Python style guide called PEP 8. This is a highly recommended reading. It gives guidelines to help writing code that is both readable and consistent. The Python community tries to follow these guidelines, more or less strictly (a key aspect of PEP 8 is that it provides guidelines and not strict rules to follow blindly).

It deals with various aspects of the code style: naming conventions, indentation convention, etc.

You'll find various tools to try to check whether your code is PEP 8 compliant and if is it not, to try and fix this:

pycodestyle package (formerly known as pep8) to check you code

pep8online to check your code with an online tool

autopep8 package to fix your code automatically

Also, this is also checked by various linters: pylint, pyflakes, flake8, etc.

There also a code formatter getting very popular in the Python communinity called Black.

In your case, various things can be modified: class names should start with an uppercase letter, whitespace can be improved, etc.

Docstring

There is also a document describing Docstring conventions called PEP 257.

The comments you've written on top of the functions definition could be used as the function docstring.

The Disc class

It seems like num is not needed. Thus, the whole class could be written:

class Disk:
 def __init__(self, size):
 self.size = size

Ultimately, we could try to see if such a class is needed at all.

Building disks

disk_num could be written in uppercase as it is a constant.

disks could be initialised using list comprehension:

# Generating disks of increasing size
DISK_NUM = 8
disks = [Disk(i) for i in range(DISK_NUM)]

Building rods

You could use keyword arguments to provide only the position value without the need to provide the disks value (and thus, rely on the default behavior). Another option could be to change the argument order to have the position first - by the way, maybe "name" would be better than "position" as it conveys better the fact that it is not an integer used for computation but just a string just for logging.

rods.append(Rod(position="Left"))
rods.append(Rod(position="Middle"))
rods.append(Rod(position="Right"))

We could once again use list comprehension to define the rods here:

# Generating rods
rods = [Rod(position=p) for p in ("Left", "Middle", "Right")]

Comparison to boolean

You don't need to write: if announce==True, you can simply write: if announce.

Rewrite logic in add_to_top

You could use elif to make the various cases easier to identify and remove a level of indentation in the second part of the function.

def add_to_top(self,current_disk,announce=True):
 """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
 if len(self.diskslist) == 0:
 if announce:
 print(f"on to the top of the self.rod_position Rod", end="n")
 self.diskslist.insert(0,current_disk)
 return True
 elif current_disk.size < self.diskslist[-1].size:
 if announce:
 print(f"on to the top of the self.rod_position Rod", end="n")
 self.diskslist.insert(0,current_disk)
 return True
 else:
 return False

It is not clearer that the 2 first cases are identical and can be merged:

def add_to_top(self,current_disk,announce=True):
 """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
 if (len(self.diskslist) == 0) or (current_disk.size < self.diskslist[-1].size):
 if announce:
 print(f"on to the top of the self.rod_position Rod", end="n")
 self.diskslist.insert(0,current_disk)
 return True
 else:
 return False

Also, maybe you could throw an exception when the disk size is invalid instead of just returning False:

def add_to_top(self,current_disk,announce=True):
 """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
 if self.diskslist and current_disk.size >= self.diskslist[-1].size:
 raise Exception("Invalid disk size")

 if announce:
 print(f"on to the top of the self.rod_position Rod", end="n")
 self.diskslist.insert(0,current_disk)

Reconsider the Rod initialisation

A disk list can be provided to the Rod initialisation.

Two things are a bit awkward here:

you are using Mutable Default Arguments which is a common Gotcha in Python

the provided list may be unsorted: if we really want to provide the list, maybe we should pass it through the add_to_top logic element by element or just check that it is sorted.

It may be easier to weaker the API by getting rid of it and default to .

def __init__(self, name=""):
 self.diskslist = 
 self.rod_name = name

At this stage, the code looks like:

class Rod:
 move_count = 0

 def __init__(self, name=""):
 self.diskslist = 
 self.rod_name = name

 def remove_top(self):
 """Remove the top disk of a rod."""
 print(f"Move the disk of size self.diskslist[0].size from self.rod_name Rod ", end="")
 Rod.move_count += 1
 return self.diskslist.pop(0)

 def add_to_top(self,current_disk,announce=True):
 """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
 if self.diskslist and current_disk.size >= self.diskslist[-1].size:
 raise Exception("Invalid disk size") 

 if announce:
 print(f"on to the top of the self.rod_name Rod", end="n")
 self.diskslist.insert(0,current_disk)


class Disk:
 def __init__(self, size):
 self.size = size


def recursive_solve(current_rod, lower_range, target_rod):
 # If only moving the top disk, execute it
 if lower_range == 0:
 rods[target_rod].add_to_top(rods[current_rod].remove_top())
 # If not, keeping simplifying the recursion.
 else:
 alt_rod = 3 - current_rod - target_rod
 recursive_solve(current_rod, lower_range - 1, alt_rod)
 recursive_solve(current_rod, 0, target_rod)
 recursive_solve(alt_rod, lower_range - 1, target_rod)


# Generating disks of increasing size
DISK_NUM = 8
disks = [Disk(i) for i in range(DISK_NUM)]

# Generating rods
rods = [Rod(name=p) for p in ("Left", "Middle", "Right")]

# Attaching all disks to the left rod
for i in range(len(disks)):
 rods[0].add_to_top(disks[len(disks)-i-1],False)


print(f"The steps needed to move DISK_NUM pieces of disks are:")
recursive_solve(0,len(disks)-1,2)

for i in range(len(rods)):
 print(f'n For rod number i+1:',end=" ")
 for j in range(len(rods[i].diskslist)):
 print(rods[i].diskslist[j].size,end=" ")

print(f'n The number of moves taken in total is Rod.move_count')

Loop with the proper tools

I highly recommend Ned Batchelder's talk "Loop like a native". Basically, anytime you use something like for xxx in range(len(yyy)), there is a better way to do it. In your case, enumerate can help you as well to write:

# Attaching all disks to the left rod
for d in reversed(disks):
 rods[0].add_to_top(d, False)

print(f"The steps needed to move DISK_NUM pieces of disks are:")
recursive_solve(0,len(disks)-1,2)

for i, rod in enumerate(rods):
 print(f'n For rod number i+1:',end=" ")
 for d in rod.diskslist:
 print(d.size,end=" ")

Also, we could consider that we can refer to rod by their name instead of their positions and thus get rid of positions altogether:

for rod in rods:
 print(f'n For rod rod.rod_name:',end=" ")
 for d in rod.diskslist:
 print(d.size,end=" ")

I have to stop here, I'll try to continue but the fact that you access rods from the recursive_solve function seems a bit weird to me.

Carcigenicate 2,37811228 · Answer 2 · 2018-10-15 12:52:15Z

Just a couple minor suggestions unrelated to the algorithm:

In a couple places, you have:

if announce==True:

This is redundant though. if is just checking if a value is truthy or not. If annouce is equal to True, that means it's already truthy. Just write:

if announce:

You'd only need == True if announce could be other values other than True or False, and you want to make sure that it's actually equal to True and not just truthy. Having a variable take on different types though is arguably poor style in most cases though, so it's rare that you'd ever need to write == True explicitly.

Along that same vein, you have:

if len(self.diskslist) == 0:

Which is also arguably verbose.

If you open a REPL and run:

bool()

You'll see that it evaluates to False. Empty collections are falsey in Python. Instead of checking the length, you could write:

if self.diskslist: # True if non-empty

Or

if not self.diskslist: # True if empty

If you don't want to need to reverse your conditional bodies

score 2 · Answer 3 · 2018-10-15 13:16:42Z

Your code seems to be working and trying to use OOP is a nice touch.
Let's see what can be improved.

Style

There is an official standard Python style guide called PEP 8. This is a highly recommended reading. It gives guidelines to help writing code that is both readable and consistent. The Python community tries to follow these guidelines, more or less strictly (a key aspect of PEP 8 is that it provides guidelines and not strict rules to follow blindly).

It deals with various aspects of the code style: naming conventions, indentation convention, etc.

You'll find various tools to try to check whether your code is PEP 8 compliant and if is it not, to try and fix this:

pycodestyle package (formerly known as pep8) to check you code

pep8online to check your code with an online tool

autopep8 package to fix your code automatically

Also, this is also checked by various linters: pylint, pyflakes, flake8, etc.

There also a code formatter getting very popular in the Python communinity called Black.

In your case, various things can be modified: class names should start with an uppercase letter, whitespace can be improved, etc.

Docstring

There is also a document describing Docstring conventions called PEP 257.

The comments you've written on top of the functions definition could be used as the function docstring.

The Disc class

It seems like num is not needed. Thus, the whole class could be written:

class Disk:
 def __init__(self, size):
 self.size = size

Ultimately, we could try to see if such a class is needed at all.

Building disks

disk_num could be written in uppercase as it is a constant.

disks could be initialised using list comprehension:

# Generating disks of increasing size
DISK_NUM = 8
disks = [Disk(i) for i in range(DISK_NUM)]

Building rods

You could use keyword arguments to provide only the position value without the need to provide the disks value (and thus, rely on the default behavior). Another option could be to change the argument order to have the position first - by the way, maybe "name" would be better than "position" as it conveys better the fact that it is not an integer used for computation but just a string just for logging.

rods.append(Rod(position="Left"))
rods.append(Rod(position="Middle"))
rods.append(Rod(position="Right"))

We could once again use list comprehension to define the rods here:

# Generating rods
rods = [Rod(position=p) for p in ("Left", "Middle", "Right")]

Comparison to boolean

You don't need to write: if announce==True, you can simply write: if announce.

Rewrite logic in add_to_top

You could use elif to make the various cases easier to identify and remove a level of indentation in the second part of the function.

def add_to_top(self,current_disk,announce=True):
 """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
 if len(self.diskslist) == 0:
 if announce:
 print(f"on to the top of the self.rod_position Rod", end="n")
 self.diskslist.insert(0,current_disk)
 return True
 elif current_disk.size < self.diskslist[-1].size:
 if announce:
 print(f"on to the top of the self.rod_position Rod", end="n")
 self.diskslist.insert(0,current_disk)
 return True
 else:
 return False

It is not clearer that the 2 first cases are identical and can be merged:

def add_to_top(self,current_disk,announce=True):
 """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
 if (len(self.diskslist) == 0) or (current_disk.size < self.diskslist[-1].size):
 if announce:
 print(f"on to the top of the self.rod_position Rod", end="n")
 self.diskslist.insert(0,current_disk)
 return True
 else:
 return False

Also, maybe you could throw an exception when the disk size is invalid instead of just returning False:

def add_to_top(self,current_disk,announce=True):
 """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
 if self.diskslist and current_disk.size >= self.diskslist[-1].size:
 raise Exception("Invalid disk size")

 if announce:
 print(f"on to the top of the self.rod_position Rod", end="n")
 self.diskslist.insert(0,current_disk)

Reconsider the Rod initialisation

A disk list can be provided to the Rod initialisation.

Two things are a bit awkward here:

you are using Mutable Default Arguments which is a common Gotcha in Python

the provided list may be unsorted: if we really want to provide the list, maybe we should pass it through the add_to_top logic element by element or just check that it is sorted.

It may be easier to weaker the API by getting rid of it and default to .

def __init__(self, name=""):
 self.diskslist = 
 self.rod_name = name

At this stage, the code looks like:

class Rod:
 move_count = 0

 def __init__(self, name=""):
 self.diskslist = 
 self.rod_name = name

 def remove_top(self):
 """Remove the top disk of a rod."""
 print(f"Move the disk of size self.diskslist[0].size from self.rod_name Rod ", end="")
 Rod.move_count += 1
 return self.diskslist.pop(0)

 def add_to_top(self,current_disk,announce=True):
 """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
 if self.diskslist and current_disk.size >= self.diskslist[-1].size:
 raise Exception("Invalid disk size") 

 if announce:
 print(f"on to the top of the self.rod_name Rod", end="n")
 self.diskslist.insert(0,current_disk)


class Disk:
 def __init__(self, size):
 self.size = size


def recursive_solve(current_rod, lower_range, target_rod):
 # If only moving the top disk, execute it
 if lower_range == 0:
 rods[target_rod].add_to_top(rods[current_rod].remove_top())
 # If not, keeping simplifying the recursion.
 else:
 alt_rod = 3 - current_rod - target_rod
 recursive_solve(current_rod, lower_range - 1, alt_rod)
 recursive_solve(current_rod, 0, target_rod)
 recursive_solve(alt_rod, lower_range - 1, target_rod)


# Generating disks of increasing size
DISK_NUM = 8
disks = [Disk(i) for i in range(DISK_NUM)]

# Generating rods
rods = [Rod(name=p) for p in ("Left", "Middle", "Right")]

# Attaching all disks to the left rod
for i in range(len(disks)):
 rods[0].add_to_top(disks[len(disks)-i-1],False)


print(f"The steps needed to move DISK_NUM pieces of disks are:")
recursive_solve(0,len(disks)-1,2)

for i in range(len(rods)):
 print(f'n For rod number i+1:',end=" ")
 for j in range(len(rods[i].diskslist)):
 print(rods[i].diskslist[j].size,end=" ")

print(f'n The number of moves taken in total is Rod.move_count')

Loop with the proper tools

I highly recommend Ned Batchelder's talk "Loop like a native". Basically, anytime you use something like for xxx in range(len(yyy)), there is a better way to do it. In your case, enumerate can help you as well to write:

# Attaching all disks to the left rod
for d in reversed(disks):
 rods[0].add_to_top(d, False)

print(f"The steps needed to move DISK_NUM pieces of disks are:")
recursive_solve(0,len(disks)-1,2)

for i, rod in enumerate(rods):
 print(f'n For rod number i+1:',end=" ")
 for d in rod.diskslist:
 print(d.size,end=" ")

Also, we could consider that we can refer to rod by their name instead of their positions and thus get rid of positions altogether:

for rod in rods:
 print(f'n For rod rod.rod_name:',end=" ")
 for d in rod.diskslist:
 print(d.size,end=" ")

I have to stop here, I'll try to continue but the fact that you access rods from the recursive_solve function seems a bit weird to me.

Carcigenicate 2,37811228 · Answer 4 · 2018-10-15 12:52:15Z

Just a couple minor suggestions unrelated to the algorithm:

In a couple places, you have:

if announce==True:

This is redundant though. if is just checking if a value is truthy or not. If annouce is equal to True, that means it's already truthy. Just write:

if announce:

You'd only need == True if announce could be other values other than True or False, and you want to make sure that it's actually equal to True and not just truthy. Having a variable take on different types though is arguably poor style in most cases though, so it's rare that you'd ever need to write == True explicitly.

Along that same vein, you have:

if len(self.diskslist) == 0:

Which is also arguably verbose.

If you open a REPL and run:

bool()

You'll see that it evaluates to False. Empty collections are falsey in Python. Instead of checking the length, you could write:

if self.diskslist: # True if non-empty

Or

if not self.diskslist: # True if empty

If you don't want to need to reverse your conditional bodies

Search This Blog

Iyfjky

Tower of Hanoi Recursive Implementation using Python with OOP

2 Answers
2

Your Answer

Post as a guest

2 Answers
2

2 Answers
2

Post as a guest

Comments

Post a Comment

Popular posts from this blog

What does second last employer means? [closed]

List of Gilmore Girls characters

Confectionery

Category

Random preview

Tower of Hanoi Recursive Implementation using Python with OOP

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Post as a guest

2 Answers 2

2 Answers 2

Sign up or log in

Post as a guest

Post as a guest

Sign up or log in

Post as a guest

Sign up or log in

Post as a guest

Sign up or log in

Post as a guest

Comments

Post a Comment

Popular posts from this blog

What does second last employer means? [closed]

List of Gilmore Girls characters

Confectionery

2 Answers
2

2 Answers
2

2 Answers
2