1
\$\begingroup\$

I'm learning about the command design pattern and would like you to critique it for division of responsibility, especially with regards to how the robot "undoes" commands it previously executed and where those commands are stored (inside RobotController)

Everything seems to work correctly.

I do have a question:

If I were to create another receiver (say, FlyingRobot), is it customary to create another set of concrete commands for it? What would I have to change if I wanted receivers to share some commands but not others?

from collections import deque, namedtuple

# Invoker
class RobotController(object):
    inst_count = 0
    def __init__(self, name=None, commands=None):
        if not name:
            self.name = f"RobotController #{RobotController.inst_count}"
        else:
            self.name = name
        if not commands:
            self._commands = deque()
        else:
            self._commands = commands
        self.previously_executed_cmds = deque()
        RobotController.inst_count += 1

    def __str__(self):
        return self.name

    @property
    def commands(self):
        return self._commands

    @commands.setter
    def commands(self, other):
        for cmd in other:
            self._commands.append(cmd)

    # Batch-Execute
    def ExecuteCommand(self, batch_size=None):
        '''Execute commands from queue of commands, up to batch_size number of commands'''
        if not batch_size:
            batch_size = len(self.commands)

        execute_results = deque()
        while len(self.commands) >= 1 and batch_size > 0:
            cmd = self.commands.popleft() # Get the next command in the queue
            execute_results.append((cmd, cmd.Execute()))
            self.previously_executed_cmds.append(cmd)
            batch_size -= 1
        return execute_results

    def UndoCommands(self):
        '''Execute previously executed command but in reverse.'''
        while self.previously_executed_cmds:
            cmd = self.previously_executed_cmds.pop()
            cmd.Undo()

# Receiver
class Robot(object):
    inst_count = 0
    supportedMoveAction = ["Forward", "Backwards", "Left", "Right", "Stop", "No-Op"]
    def __init__(self, name=None):
        if not name:
            self.name = f"Robot #{Robot.inst_count}"
        else:
            self.name = name
        Robot.inst_count += 1

    def __str__(self):
        return self.name

    def Move(self, moveAction, distance):
        if moveAction in Robot.supportedMoveAction:
            actiontaken = f'{self}: Moving [{moveAction}, {distance}].'
        else:
            actiontaken = f'{self}: Move Failed: Unsupported move action {moveAction}.'
        print(actiontaken) # Perform the action
        return actiontaken

# Command (Base/Abstract)
class RobotCommand(object):
    def __init__(self):
        pass
    # API Common to all Command subclasses
    def Execute(self):
        pass # Performs a Robot Command
    def Undo(self):
        pass # Reverts Robot to the state it was in before .Execute() was called

# Command (Concrete): Aware of the Receiver object's API
class Move(RobotCommand):
    undo_movement = {'Forward':'Backwards', 'Backwards':'Forward', 'Left':'Right', 'Right':'Left', 'Stop':'Stop', 'No-Op':'No-Op'}

    def __init__(self, receiver, action="No-Op", distance=0):
        self.receiver = receiver
        self.action = action
        self.distance = distance

    def Execute(self):
        return self.receiver.Move(self.action, self.distance)

    def Undo(self):
        '''
        Notice that the Move Command stores state information about: 
            1. What Receiver object was called (the specific Robot instance)
            2. The details with which to pass to Robot.Move() (i.e., `action` and `distance`)
        This necessarily relinquishes the responsibility of the Receiver to implement
        "Undo" and store state information (i.e., what it was previously told to do)
        ''' 
        self.receiver.Move(Move.undo_movement[self.action], self.distance)

# To avoid duplicating code and as a way to assign maneuvers to arbitrary robot receivers
def get_maneuver(robot_receiver):
    L_maneuver = deque([
        Move(robot_receiver, 'Forward', 5),
        Move(robot_receiver, 'Left', 4)
    ])

    cww_maneuver = deque([
        Move(robot_receiver, 'Forward', 3), 
        Move(robot_receiver, 'Left', 3),
        Move(robot_receiver, 'Backwards', 3), 
        Move(robot_receiver, 'Right', 3)
    ])
    return L_maneuver, cww_maneuver

if __name__ == '__main__':
    maneuvers = namedtuple('Maneuvers', ['LManeuver', 'CcwCircle'])

    # Create our actors
    Wallie = Robot('Wallie')
    Eve = Robot('Eve')

    # Initiatize their maneuvers
    wallie_maneuvers = maneuvers(*get_maneuver(Wallie))
    eve_maneuvers = maneuvers(*get_maneuver(Eve))

    # Initialize a controller
    Nasah = RobotController(name='Nasah')

    # Should be harmless if no commands were previously provided
    Nasah.ExecuteCommand()
    Nasah.UndoCommands()

    print('L Maneuver:')
    Nasah.commands = wallie_maneuvers.LManeuver
    Nasah.ExecuteCommand()
    Nasah.UndoCommands()

    print('3-2-1 Stroll:')
    Nasah.commands.append(Move(Wallie, 'Forward', 3))
    Nasah.commands.append(Move(Wallie, 'Forward', 2))
    Nasah.commands.append(Move(Wallie, 'Forward', 1))
    Nasah.ExecuteCommand(batch_size=1)
    Nasah.ExecuteCommand(batch_size=2)
    Nasah.UndoCommands() # Backwards 1, Backwards 2, Backwards 3

    print('Counterclockwise Circle Maneuver:')
    Nasah.commands = wallie_maneuvers.CcwCircle
    Nasah.ExecuteCommand()
    Nasah.UndoCommands()

    print('Eve\'s Turn:')
    Nasah.commands = eve_maneuvers.LManeuver
    Nasah.commands = eve_maneuvers.CcwCircle
    Nasah.UndoCommands() # Should do nothing
    Nasah.ExecuteCommand() # Eve to perform LManeuver and CcwCircle
    Nasah.UndoCommands()

Output

L Maneuver:
Wallie: Moving [Forward, 5].
Wallie: Moving [Left, 4].
Wallie: Moving [Right, 4].
Wallie: Moving [Backwards, 5].
3-2-1 Stroll:
Wallie: Moving [Forward, 3].
Wallie: Moving [Forward, 2].
Wallie: Moving [Forward, 1].
Wallie: Moving [Backwards, 1].
Wallie: Moving [Backwards, 2].
Wallie: Moving [Backwards, 3].
Counterclockwise Circle Maneuver:
Wallie: Moving [Forward, 3].
Wallie: Moving [Left, 3].
Wallie: Moving [Backwards, 3].
Wallie: Moving [Right, 3].
Wallie: Moving [Left, 3].
Wallie: Moving [Forward, 3].
Wallie: Moving [Right, 3].
Wallie: Moving [Backwards, 3].
Eve's Turn:
Eve: Moving [Forward, 5].
Eve: Moving [Left, 4].
Eve: Moving [Forward, 3].
Eve: Moving [Left, 3].
Eve: Moving [Backwards, 3].
Eve: Moving [Right, 3].
Eve: Moving [Left, 3].
Eve: Moving [Forward, 3].
Eve: Moving [Right, 3].
Eve: Moving [Backwards, 3].
Eve: Moving [Right, 4].
Eve: Moving [Backwards, 5].
\$\endgroup\$
1
\$\begingroup\$

Having the undo in the global controller object is fine. It implies that the undo action is global. I probably would have saved the state of the robot to the queue each time a command is received. Then undo is just restoring the state to what it was at a previous time. It also makes going back 'n' moves at a time much cheaper.

It is generally considered more "Pythonic" to try to execute the move in a try block and catch the resulting failure instead of having a function that tells you if the move is legal. Alternatively you can use hasattr() to check if the function you want to call is an attribute of this particular object before you call it. Raising an exception in Python is much cheaper than in some other languages and is often used if it doesn't obfuscate the code.

I would think this application is a good candidate for using Python's multiple inheritance by creating mix-in classes. These are classes that can be inherited from but cannot be instantiated on their own (since they do not have an __init__ method). So a MovingRobot would inherit from Robot (which can be instantiated and supplies the undo functionality) and Mover (which supplies state information about position). Similarly FlyingRobot would inherit from both Robot and Flyer.

\$\endgroup\$
  • \$\begingroup\$ Interesting first point. I think the choice to store Receiver states or to store Commands depends on what the Receiver actually is. If it were some class with a large number of attributes and you had defined a state-transition function (specifying how the object needs to change to get from one state to another), saving state would be a good idea. If code for the Receiver was actually located on a resource-constrained platform (e.g., a microcontroller), saving the commands on the controller-platform (which may be less resource-constrictive) is a better idea. \$\endgroup\$ – Minh Tran Jul 12 at 2:36
  • \$\begingroup\$ With a try-block, I'd have to raise an exception in Robot.Move() and wrap every call to Robot.Move() with a try-except (or write a function to take a collection of Move Commands and try each Command). The latter isn't so bad but it depends on the Receiver. If the error in the Receiver is dire enough (and so should crash the program) or if it's more concise to let the language deal with bad inputs rather than depending on the programmer to exhaustively specify what to do with bad inputs on a case-by-case basis, exceptions is the way to go. \$\endgroup\$ – Minh Tran Jul 12 at 2:55
  • \$\begingroup\$ Hadn't heard of mix-in classes. Reminds me of C# interfaces / C++ abstract classes. Great suggestion! \$\endgroup\$ – Minh Tran Jul 12 at 2:58

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.