Using Instance Attributes as Dictionary Values

Question

Pre-script: I have searched through many a thread on SO, but none seem to answer the question I have.

I have made a small script that deals with a point moving around a grid while updating a set with all those points that it has traversed. The gist of the move() method is:

# self.x and self.y are initialised to 0 at the time of object creation
# dir_ - direction in which to move
# steps - number of steps to move

def _move(self, dir_, steps):                
        if dir_ == 'U':
            for step in range(steps):
                self.y += 1
                self.locations.add((self.x, self.y))
        elif dir_ == 'R':
            for step in range(steps):
                self.x += 1
                self.locations.add((self.x, self.y))
        elif dir_ == 'L':
            for step in range(steps):
                self.x -= 1
                self.locations.add((self.x, self.y))
        elif dir_ == 'D':
            for step in range(steps):
                self.y -= 1
                self.locations.add((self.x, self.y))
        else:
            raise Exception("Invalid direction identifier.")

As you can see there's a lot of repetition. In my eagerness to clean things up I attempted something like this:

from operator import add, sub

def _move(self, dir_, steps):                
    dir_dict = {'U': (self.y, add), \
                'D': (self.y, sub), \
                'L': (self.x, sub), \
                'R': (self.x,add)}

    coord, func = dir_dict[dir_]
    for step in range(steps):
        coord = func(coord, 1)
        locations.add(self.x, self.y)

It turns out that I can't expect the references to an object's attributes be passed around like that, and as a result, that self.x and self.y weren't updated.

Questions:

1. How can I clean this code up to avoid the repetition?

2. Even if the original piece of code is deemed "not all that bad" for the functionality, is there a way to pass around instance attributes in the way I intended to?

Answer 1

In terms of refactoring, there's a lot of different ways to do it. Personally, I believe that the caller should be responsible to determining what the point should do, not the point determining what to do based on string input. The problem here is that this method doesn't make the Point really extendable (you can't change the base functionality without having to change the _move function)

So here's my take:

Initially, we can simplify the _move function like so:

def _move(self, distances, steps=1):
    """Move the Point a given distance along the x,y axis, a given amount of times and save the new location"""
    distance_x, distance_y = distances
    for step in range(steps):
        self.x += distance_x
        self.y += distance_y
        self.locations.add((self.x, self.y))     

Here's whats happening. The _move function now expects the distances along the x and y axis to move the point, and how many times to move it. distances in this case is a tuple. For clarity, it is unpacked into distance_x and distance_y variables. The distances are then added to the x and y value of the point, and then saved to the locations list. For your usecase, the caller would do the lookup for what the point should do.

if action == 'U':
    point._move(0, 1)
elif action == 'D':
    point._move(0, -1)
...

Now, if you wanted to define the specific movements the point can make, you can do the following:

def move_up(self, distance=1, steps=1):
    """Move the Point up a given distance, a given amount of times

    Precondition: The distance is positive
    """
    assert distance >= 0
    self._move((0, distance), steps)

def move_right(self, distance=1, steps=1):
    """Move the Point right a given distance, a given amount of times

    Precondition: The distance is positive
    """
    assert distance >= 0
    self._move((distance, 0), steps)

def move_down(self, distance=1, steps=1):
    """Move the Point down a given distance, a given amount of times

    Precondition: The distance is positive
    """
    assert distance <= 0 
    self._move((0, -distance), steps)

def move_left(self, distance=1, steps=1):
    """Move the Point left a given distance, a given amount of times

    Precondition: The distance is positive
    """    
    assert distance <= 0
    self._move((-distance, 0), steps)

Here, each function defines how the point will move in each direction. Each direction takes a distance allowing the caller to define how many grid spaces to move the point in a given direction, and the number steps. By default, each function moves it one in each direction. The assertions are there because it seems weird to be able to move a certain direction using a negative number (moving right -1 is the same as moving left+1), but they are not required based on your use case.

And the caller would look like so:

if action == 'U':
    point.move_up()
elif action == 'D':
    point.move_down()
...

While more verbose, there are several benefits to this approach.

1. Each direction is contained in its own function. This will allow subclasses to easily override the base point behavior. For example, if you want to track a Point that moves up 2 grid spaces per step, you could extend the BasePoint and rewrite the move_up function to look something like this:

def move_up(steps=1):
    super().move_up(distance=2, steps=steps)

2. The _move function is more concise, as it does not need to know which direction to move the Point. It simply takes the distance along the x and y axis to move, moves the point, and saves the new location. This makes the class more extensible, as you can easily create new directions for the point to move in (ie. diagonal points) by adding new functions

def move_diagonal(distance_x=1, distance_y=1, steps=1)
    super()._move(distance=(distance_x, distance_y), steps=steps)

3. The caller controls how the point is called. This allows the caller to define the rules for calling the function

if action == 'U' and len(p.locations) < TOTAL_STEPS:
    p.move_up()
else:
    raise TooManyStepsException('You have exceeded the number of allowed steps')

Hopefully this helps, and gives you a couple of different approaches based on your use case.

Answer 2

Here's how I would do it: you want your dictionary values to represent movements; those are actions which should update the object's state, so it would make sense to represent them as functions which do the state updates, as opposed to data about what state updates to do. (You are halfway there by storing add or sub as functions.)

Give the class move_up, move_down, move_left and move_right methods, and then store references to those methods in the dictionary.

    def move_up(self):
        self.y += 1
    def move_down(self):
        self.y -= 1
    def move_left(self):
        self.x -= 1
    def move_right(self):
        self.x += 1

    def _move(self, dir_, steps):
        dir_dict = {'U': self.move_up,
                    'D': self.move_down,
                    'L': self.move_left,
                    'R': self.move_right}

        func = dir_dict[dir_]
        for step in range(steps):
            func()
            self.locations.add( (self.x, self.y) )

Answer 3

Your first refactor is definitely on the right track. The issue you're seeing is that add and sub return new values, not existing ones. coord is not the same as self.x or self.y. I would use an attribute lookup here

from operator import add, sub

def _move(self, dir_, steps):                
    dir_dict = {'U': ('y', self.y, add), \
                'D': ('y', self.y, sub), \
                'L': ('x', self.x, sub), \
                'R': ('x', self.x, add)}

    attr, coord, func = dir_dict[dir_]
    for step in range(steps):
        coord = func(coord, 1)
        # set the attribute on self here
        setattr(self, attr, coord)
        locations.add(self.x, self.y)