Script for Python Hanoi Tower OOP

Question

Is my Python Script to play Hanoi Tower respectful of OOP spirit?

I have difficulties to create meaningful classes so I started an easy project. Here is one class at the plateau level and then a class to handle towers.

How can it be improved (on the OOP level and on script level)?

I want then a GUI with TKinter, a third class GUI would be suited ?

Thank you

import numpy as np
NB_DISKS = 3
class Plateau:
 """Class for the whole plateau"""
 def __init__(self):
 self.init_plateau()
 def init_plateau(self):
 """List of towers,first tower full"""
 self.nb_disks = NB_DISKS
 self.towers = [Tower(self.nb_disks), Tower(0), Tower(0)]
 def motion(self, tower_from, tower_to): 
 """Motion from one tower to another one with checking"""
 from_tower = self.towers[tower_from-1]
 to_tower = self.towers[tower_to-1]
 if from_tower.get_last_disk()>to_tower.get_last_disk(): #Receive 0 if to_tower is empty
 disk = from_tower.get_last_disk()
 from_tower.delete_disk()
 to_tower.add_disk(disk) 
 self.check_victory()
 self.print_towers()
 else:
 print('Last disk number from origin has to be bigger than reception one')
 def check_victory(self):
 """Check if last tower has an ordered list or not"""
 last_tower = self.towers[2].get_whole_tower()
 if len(last_tower) == self.nb_disks:
 diff_last_tower = np.diff(last_tower)
 if np.unique(diff_last_tower) == 1: 
 print('victory:') 
 self.print_towers()
 print('new plateau:')
 self.init_plateau() 
 def print_towers(self):
 """Print the towers"""
 for elt in self.towers:
 elt.print_tower()
 print('\n')
class Tower:
 """Class for a tower"""
 def __init__(self,nb_disks):
 """Creation of a tower"""
 if nb_disks !=0:
 self.tower = list(range(1, nb_disks+1))
 else:
 self.tower = []
 def get_last_disk(self):
 """Return last element of the tower"""
 if self.tower == []:
 return 0
 else:
 return self.tower[-1]
 def delete_disk(self):
 """Delete last disk of the tower"""
 self.tower.pop()
 def add_disk(self,disk):
 """Add a disk to the top of the tower"""
 self.tower.append(disk)
 def get_whole_tower(self):
 """Return the complete tower"""
 return(self.tower)
 def print_tower(self): 
 """Print the element of the tower"""
 print(self.tower)
if __name__ == '__main__':
 game = Plateau()
 game.motion(1,3)
 game.motion(1,3) # Try to move a big disk on a small one 
 game.motion(1,2)
 game.motion(3,2) 
 game.motion(1,3)
 game.motion(2,1) 
 game.motion(2,3) 
 game.motion(1,3) 

Answer 1

From the OOP/typing level, you might want to consider having Disk be a type. If it doesn't need any methods, you could just make it a subtype of int:

from typing import NewType
Disk = NewType('Disk', int)

Some notes on your interfaces:

1. Since you always use get_last_disk and delete_disk together, why not combine them? The thing you really want is a pop_top_disk operation that removes and returns the last disk (that's already how you're implementing delete_disk anyway).

2. add_disk could implement the "no larger disk on a smaller one" rule.

3. get_whole_tower seems bad because it's exposing the internal data structure of the tower; this violates your entire OOP abstraction. Note also that in general you should distinguish between "private" and "public" members of your classes.

4. Rather than making NB_DISKS a global and having Plateau initialize itself from that, Plateau's initializer should take the number of disks as a parameter.

5. The Pythonic way of making a "print" function is to implement the magic function __repr__ to return a string.

6. The int values of your disks should be reversed so that bigger numbers represent bigger disks.

7. This is more an English thing than a Python thing, but the verb form of "motion" is "move" so it would be better to name your method move. :)

Here's the final code I came up with after implementing the above suggestions:

from typing import NewType
Disk = NewType('Disk', int)
class WrongOrder(Exception):
 def __init__(self, top: int, bottom: int) -> None:
 super().__init__("Can't put a disk of size %d on a disk of size %d" % (top, bottom))
class Plateau:
 """Class for the whole plateau"""
 def __init__(self, nb_disks: int) -> None:
 self._init_plateau(nb_disks)
 def _init_plateau(self, nb_disks: int) -> None:
 self._towers = [Tower(nb_disks), Tower(0), Tower(0)]
 def move(self, from_tower: int, to_tower: int) -> None: 
 """Move from one tower to another (towers specified as an int from 1-3). 
 Prints an error if the move is invalid."""
 # Convert from 1-index to 0-index.
 from_tower -= 1
 to_tower -= 1
 # Make the move, print exception if it fails.
 try:
 disk = self._towers[from_tower].pop_top_disk()
 try:
 self._towers[to_tower].add_disk(disk)
 except WrongOrder:
 # don't drop the disk! Put it back where we got it and reraise.
 self._towers[from_tower].add_disk(disk)
 raise
 except Exception as e:
 print('Move failed: ', str(e))
 else:
 self._check_victory()
 print(self)
 def _check_victory(self) -> None:
 """Check if all disks have moved to the last tower (victory condition).
 If the player has achieved victory, reset the game.
 """
 if sum(tower.height for tower in self._towers) == self._towers[2].height:
 print('victory:') 
 print(self)
 print('new plateau:')
 self._init_plateau(self._towers[2].height) 
 def __repr__(self) -> str:
 """Print the towers"""
 return "\n".join([repr(tower) for tower in self._towers]) + "\n"
class Tower:
 """Class for a tower"""
 def __init__(self, nb_disks: int) -> None:
 """Creation of a tower"""
 self._tower = [Disk(size) for size in range(1, nb_disks + 1)]
 self._tower.reverse() # order the stack from largest to smallest
 def pop_top_disk(self) -> Disk:
 """Remove and return the top disk on this tower."""
 return self._tower.pop()
 def add_disk(self, disk: Disk) -> None:
 """Add a disk to the top of the tower.
 Raises WrongOrder if the disk being added is too big.
 """
 if len(self._tower) and self._tower[-1] < disk:
 raise WrongOrder(disk, self._tower[-1])
 self._tower.append(disk)
 @property
 def height(self) -> int:
 """Number of disks in the tower."""
 return len(self._tower)
 def __repr__(self) -> str: 
 """Print the elements of the tower"""
 return repr(self._tower)
if __name__ == '__main__':
 game = Plateau(3)
 game.move(1,3)
 game.move(1,3) # Try to move a big disk on a small one 
 game.move(1,2)
 game.move(3,2) 
 game.move(1,3)
 game.move(2,1) 
 game.move(2,3) 
 game.move(1,3)

• ShapeOfMatter: This is mostly good. I may add an answer of my own, but I wanted to react to your point #1: In OP's code, they did not always use get_last_disk and delete_disk together, because they weren't treating illegal moves as exceptional. Between your nested-try pattern and what OP wrote, I think I'd prefer the original!
• Kerdiorp: Thanks, you provided me nice advices on POO and Python. I will look more for @Property, repr and exception handling. NewType seems also to be a nice feature

Answer 2

The "spirit" of Object Oriented Programming is two-fold:

• To give us an enforceable metaphor for reasoning about our code. ("objects" as objects)
• To give us heuristics about how to compartmentalize out code. (encapsulation)

The purist perspective of OOP can be a bit verbose, and in python it's not usually necessary.

I want then a GUI with TKinter, a third class GUI would be suited ?

Try thinking about it like this: An object method will have both a method that it's called on and a context in which it gets called. If you build a class to encapsulate your GUI, where will it's methods get called from?

One of the principal things objects do is manage state. If a function needs to update state, that suggests that the function and the state could be encapsulated together in an object. If the function just needs to read a state, and that state (data) could be passed in as an argument by the calling context, then the function can be a static method or a global function. If a class only has static methods, then you don't need a class at all. That's a good thing: the less state you're managing the less opportunity to mess it up.

How can it be improved (on the OOP level and on script level)?

Sam Stafford's points #4 and #5 are good, as is the suggestion to have Disk as a new type.

1. You could also neglect to declare a proper class for the Towers by just having Tower = NewType('Tower', List(Disk)).
2. If you're thinking of the normal input and output as a user-interface, then you probably shouldn't be printing (or reading input) from inside class methods. That said, logging is a fine thing to do, and print is a low-effort way to do it.
3. Plateau.motion() does too many things. Checking for victory should certainly go outside in the calling context. I would suggest that validating the user-input also doesn't belong in there.
4. Similarly, Plateau.check_victory() shouldn't set up the new game, and init_plateau should get inlined into Plateau.__init__(). When you start a new game just build a new Plateau.

Taken to the extreme, you could have a static class representing the state of the game, and a function to start a new game, and then you'd repeatedly call a function (GameState, PlayerMove)->GameState. At that point you'd be breaking past traditional imperative OOP into a more "functional" style.