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.
- 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)
- 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)
- 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.
dir_dict = {'U': (0, 1), 'D': (0, -1), ... }