2
\$\begingroup\$

I created this program some time ago. It shows you how to solve tower of hanoi for n disks, step by step.

Here's the code:

from os import system
from colorama import Fore, Style

clear = lambda: system('cls')

class TowerOfHanoi:
    def __init__(self, n, r1, r2, r3):
        self.n = n

        self.r1 = r1
        self.r2 = r2
        self.r3 = r3

        clear()
        print('Initial: ')
        self.parser(a, b, c)

        input('\nEnter To Continue...')

        self.solve()

    def parser(self, a1, b1, c1):
        x, y, z = [], [], []

        a1, b1, c1 = [0] * (self.n - len(a1)) + a1, \
                     [0] * (self.n - len(b1)) + b1, \
                     [0] * (self.n - len(c1)) + c1

        m = self.n

        for i in a1:
            s = ' ' * (m - i + 1)
            h = Fore.RED + s + '██' * i + s + Style.RESET_ALL

            x.append(h)

        for i in b1:
            s = ' ' * (m - i + 1)
            h = Fore.RED + s + '██' * i + s + Style.RESET_ALL

            y.append(h)

        for i in c1:
            s = ' ' * (m - i + 1)
            h = Fore.RED + s + '██' * i + s + Style.RESET_ALL

            z.append(h)

        print(' ' * self.n + str(self.r1)
              + ' ' * (self.n * 2 + 1) + str(self.r2)
              + ' ' * (self.n * 2 + 1) + str(self.r3))

        print()

        print('\n'.join(i + ' ' * (self.n - len(i))
                + j + ' ' * (self.n - len(j))
                + k for i, j, k in zip(x, y, z)))
        print()

    def solve(self, n = None, r1 = None, r2 = None, r3 = None, x = 0, y = 1, z = 2):
        if n is None:
            n = self.n

        if r1 is None:
            r1 = self.r1
        if r2 is None:
            r2 = self.r2
        if r3 is None:
            r3 = self.r3

        if n == 1:
            clear()
            print("Move disk 1 from rod", r1, "to rod", r2)
            print()
            [a, b, c][y].insert\
                (0, [a, b, c][x].pop(0))
            self.parser(a, b, c)
            return

        self.solve(n - 1, r1, r3, r2, x, z, y)
        input('\nEnter To Continue...')

        clear()

        print("Move disk", n, "From Rod", r1, "To Rod", r2)
        print()

        [a, b, c][y].insert\
            (0, [a, b, c][x].pop(0))

        self.parser(a, b, c)

        input('\nEnter To Continue...')
        print()
        self.solve(n - 1, r3, r2, r1, z, y, x)

print('Welcome to Tower of Hanoi!')
print('You have some number of rings of increasing length')
print('Like this:')
print()
print(Fore.RED + '  ██  ' + Style.RESET_ALL)
print(Fore.RED + ' ████ ' + Style.RESET_ALL)
print(Fore.RED + '██████' + Style.RESET_ALL)
print()
print('The rules are simple: ')
print('  * You can move a disk from a rod to another rod')
print('      if the current disk is smaller than the topmost disk in the rod you are moving')
print()
print('  * The goal is to move all disks from rod 1 to rod 2')
print()
print('This program shows you how to move the disks from rod 1 to rod 2')
print()

while True:
    input('Press enter to start!')
    print()

    n1 = int(input('Enter number of rings: '))

    a = [i for i in range(1, n1 + 1)]
    b = []
    c = []

    toh = TowerOfHanoi(n1, 1, 2, 3)

    print()
    play_again = input('Do you want to play again? <Y>es or <N>o').lower()

    while play_again != 'y' or play_again != 'n':
        play_again = input('Do you want to play again? <Y>es or <N>o').lower()

    if play_again == 'n':
        print('Bye!')
        break

This code seems a bit lengthy, as well as ugly. How do I improve it?

Any tips are welcome!

\$\endgroup\$
2
\$\begingroup\$

Bugs

Colorama Not Initialized

Colorama should be initialized, to ensure proper operation:

import colorama

colorama.init()

Clear Screen

sh: cls: command not found

On non-Windows machines, cls is not defined

Instead, simply use colorama to clear the screen.

def clear():
    print(colorama.ansi.clear_screen())

Play again

Do you want to play again? <Y>es or <N>oY 
Do you want to play again? <Y>es or <N>oN 
Do you want to play again? <Y>es or <N>oy 
Do you want to play again? <Y>es or <N>on 
Do you want to play again? <Y>es or <N>o 

No answer seems to provide the expected behaviour, due to:

while play_again != 'y' or play_again != 'n':

always being True. You need the and conjunction, not or.


Unused variables

toh = TowerOfHanoi(n1, 1, 2, 3)

toh is never used after this point. So, why store it in a variable?


Global variables

a = [i for i in range(1, n1 + 1)]
b = []
c = []

toh = TowerOfHanoi(n1, 1, 2, 3)

a, b, and c are used inside the TowerOfHanoi class, but are not passed to the class. Instead they are globals variables that must be properly initialized corresponding to the value n1 passed to the TowerOfHanoi constructor.

These lists should be created by the class itself, based on the argument passed to the constructor. Moreover, they should be members of the class, not globals.


Code Duplication

This code:

    for i in a1:
        s = ' ' * (m - i + 1)
        h = Fore.RED + s + '██' * i + s + Style.RESET_ALL

        x.append(h)

is replicated 3 times, with a1 changing to b1 and c1, and x changing to y and z. Perhaps you could refactor this into its own function, taking the rod list as input, and generating & returning the lists internally?

    def rod_images(self, rod):
        # Fill rod with required 0's at top
        rod = [0] * (self.n - len(rod)) + rod

        image_rows = []
        for i in rod:
            spaces = ' ' * (self.n - i + 1)
            row = Fore.RED + spaces + '██' * i + spaces + Style.RESET_ALL
            image_rows.append(row)

        return image_rows

Then you could simply write:

    def parser(self, a1, b1, c1):

        x = self.rod_images(a1)
        y = self.rod_images(b1)
        z = self.rod_images(c1)

        # ... etc ...

Zero, One & Many

In programming, there are 3 important numbers: zero, one, and many. How many rods do you have? More than one, so ... you have many rods. Many items are stored in containers, such as lists, not individual variables. So instead of this:

    self.r1 = r1
    self.r2 = r2
    self.r3 = r3

write something like:

    self.rods = [r1, r2, r3]

Then, instead of

    print(' ' * self.n + str(self.r1)
          + ' ' * (self.n * 2 + 1) + str(self.r2)
          + ' ' * (self.n * 2 + 1) + str(self.r3))

you can take advantage of the fact that you are doing something with each item in the list, in order. In this case, you are converting each item to a string, and then joining them together with a bunch of spaces in-between:

    print(' ' * self.n
          + (' ' * (self.n * 2 + 1)).join(str(rod) for rod in self.rods)

But ... that still looks ugly. What you are really doing is printing out 3 values, each centred in a n * 2 + 2 character wide field:

    width = self.n * 2 + 2
    print((f"{{:^{width}}}"*3).format(*self.rods))

With 3 disks, this creates the format string "{:^8}{:^8}{:^8}" which centres the 3 values each in 8 character wide fields.

A similar method could be used to centre the discs when printing, as long as the colour/style codes are removed from the disc "values" and moved to the format string itself, so they don't mess up the character counts for automatic centring.

Unnecessary Default Arguments

Calling solve(), with no arguments, is only done from __init__(). So the only time the default arguments are used is that one call. So why complicate the method with default arguments which must be set via a number of if _ is None: tests? Simply use:

self.solve(self.n, self.r1, self.r2, self.r3, 0, 1, 2)

in the __init__() method, and remove the default arguments and default argument substitution code.

\$\endgroup\$
  • \$\begingroup\$ Can you please explain why colorama.init() have to be used? I was using them for some time, but then realized it works without colorama.init() as well. \$\endgroup\$ – Srivaths Nov 14 at 3:23
  • \$\begingroup\$ From the documentation, "On Windows, calling init() will filter ANSI escape sequences out of any text sent to stdout or stderr, and replace them with equivalent Win32 calls." Based on your system('cls') function, I expect you are on Windows. Perhaps you're running in a Windows terminal window that supports ANSI escape sequences already. If you share your script with others, and they run it in a different environment, it may stop working if colorama.init() is not called. It is never wrong to call the initialization code, even if it becomes a no-op. \$\endgroup\$ – AJNeufeld Nov 14 at 18:24

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.