3
\$\begingroup\$

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)      
\$\endgroup\$
4
\$\begingroup\$

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)
\$\endgroup\$
2
  • \$\begingroup\$ 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! \$\endgroup\$ – ShapeOfMatter Dec 24 '19 at 20:02
  • \$\begingroup\$ 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 \$\endgroup\$ – Kerdiorp Dec 25 '19 at 10:16
4
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Ok thanks, I will focus on writing smaller methods \$\endgroup\$ – Kerdiorp Dec 25 '19 at 10:20

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.