Let’s see if we can make PathHelper less bad, perhaps even make it better.

Here’s PathHelper now:

class PathHelper:
    def __init__(self, space, source, target):
        self.space = space
        self.source = source
        self.target = target

    def compute_via(self):
        sx, sy = self.source.xy
        tx, ty = self.target.xy
        dx = tx - sx
        dy = ty - sy
        length = math.sqrt(dx*dx + dy*dy)
        theta = math.atan2(dy, dx)
        up_offset = self.rotate((length//3, 5), theta)
        up = self.source.at_offset(*up_offset)
        dn_offset = self.rotate((2*length//3, -5), theta)
        dn = self.source.at_offset(*dn_offset)
        return [up, dn]

    @staticmethod
    def rotate(xy, angle):
        x, y = xy
        x_rot = x * math.cos(angle) - y * math.sin(angle)
        y_rot = x * math.sin(angle) + y * math.cos(angle)
        return int(x_rot), int(y_rot)

We’d like to make it a bit nicer, though it’s not really awful. And to use it, we need to ensure that both the new points in the via are available. We’ll write a test for that in due time.

We begin by moving the PathHelper class to live with Cell, since it is a helper to that class. Maybe we’ll move it again: I’m not sure.

Let’s begin by breaking out the three obvious pieces of the compute_via method into separate methods. First this:

class PathHelper:

    def compute_via(self):
        length, theta = self.get_length_and_theta()
        up_offset = self.rotate((length//3, 5), theta)
        up = self.source.at_offset(*up_offset)
        dn_offset = self.rotate((2*length//3, -5), theta)
        dn = self.source.at_offset(*dn_offset)
        return [up, dn]

    def get_length_and_theta(self) -> tuple[float, float]:
        sx, sy = self.source.xy
        tx, ty = self.target.xy
        dx = tx - sx
        dy = ty - sy
        length = math.sqrt(dx * dx + dy * dy)
        theta = math.atan2(dy, dx)
        return length, theta

Now I think we really just want to use the fraction along the line, so let’s extract a couple of interesting variables.

    def compute_via(self):
        length, theta = self.get_length_and_theta()
        one = length
        uptick = 5
        up_offset = self.rotate((one // 3, uptick), theta)
        up = self.source.at_offset(*up_offset)
        two = 2 * length
        dn_offset = self.rotate((two // 3, -uptick), theta)
        dn = self.source.at_offset(*dn_offset)
        return [up, dn]

I’m going around the horn here so as to try to use only machine refactorings. We do have a test that is running but even so, machine refactorings are safer than typing.

Now I think if we extract a method, something good will happen:

    def compute_via(self):
        length, theta = self.get_length_and_theta()
        one = length
        uptick = 5
        up = self.rotate_offset(one, uptick, theta)
        two = 2 * length
        dn_offset = self.rotate((two // 3, -uptick), theta)
        dn = self.source.at_offset(*dn_offset)
        return [up, dn]

    def rotate_offset(self, one, uptick, theta):
        up_offset = self.rotate((one // 3, uptick), theta)
        up = self.source.at_offset(*up_offset)
        return up

OK, either I am mistaken, or PyCharm didn’t recognize the chance to do the second use of the new method. I’ll type it in, see what happens.

    def compute_via(self):
        length, theta = self.get_length_and_theta()
        one = length
        uptick = 5
        up = self.rotate_offset(one, uptick, theta)
        two = 2 * length
        dn = self.rotate_offset(two, -uptick, theta)
        return [up, dn]

Now inline the one and two and rename uptick to offset:

    def compute_via(self):
        length, theta = self.get_length_and_theta()
        offset = 5
        up = self.rotate_offset(length, offset, theta)
        dn = self.rotate_offset(2 * length, -offset, theta)
        return [up, dn]

Green. Let’s commit: refactoring.

I don’t like having the three inside after all, pull it out, do some renaming:

    def compute_via(self):
        length, theta = self.get_length_and_theta()
        offset = 5
        up = self.rotate_offset(length//3, offset, theta)
        dn = self.rotate_offset(2 * length//3, -offset, theta)
        return [up, dn]

    def rotate_offset(self, x_offset, y_offset, theta):
        rotated_offset = self.rotate((x_offset, y_offset), theta)
        cell = self.source.at_offset(*rotated_offset)
        return cell

Rename offset in compute_via. Here’s the class so far:

class PathHelper:
    def __init__(self, space, source, target):
        self.space = space
        self.source = source
        self.target = target

    def compute_via(self):
        length, theta = self.get_length_and_theta()
        y_offset = 5
        up = self.rotate_offset(length//3, y_offset, theta)
        dn = self.rotate_offset(2 * length//3, -y_offset, theta)
        return [up, dn]

    def rotate_offset(self, x_offset, y_offset, theta):
        rotated_offset = self.rotate((x_offset, y_offset), theta)
        cell = self.source.at_offset(*rotated_offset)
        return cell

    def get_length_and_theta(self) -> tuple[float, float]:
        sx, sy = self.source.xy
        tx, ty = self.target.xy
        dx = tx - sx
        dy = ty - sy
        length = math.sqrt(dx * dx + dy * dy)
        theta = math.atan2(dy, dx)
        return length, theta

    @staticmethod
    def rotate(xy, angle):
        x, y = xy
        x_rot = x * math.cos(angle) - y * math.sin(angle)
        y_rot = x * math.sin(angle) + y * math.cos(angle)
        return int(x_rot), int(y_rot)

Commit: refactoring.

Reflection

There’s more magic going on between tuple and tuple component than I like, with the dual assignments to break things up and the *rotated_offset to unpack for at_offset.

However, that’s down a couple of levels inside PathHelper, and the method name makes it clear what we’re doing, I think. And rotated_offset isn’t bad at all.

I think we should refactor rotate to accept separate x and y, like this:

    def rotate_offset(self, x_offset, y_offset, theta):
        rotated_offset = self.rotate(x_offset, y_offset, theta)
        cell = self.source.at_offset(*rotated_offset)
        return cell

    @staticmethod
    def rotate(x, y, angle):
        x_rot = x * math.cos(angle) - y * math.sin(angle)
        y_rot = x * math.sin(angle) + y * math.cos(angle)
        return int(x_rot), int(y_rot)

Commit. Refactor:

    def rotate_offset(self, x_offset, y_offset, theta):
        x_rot, y_rot = self.rotate(x_offset, y_offset, theta)
        cell = self.source.at_offset(x_rot, y_rot)
        return cell

Commit. That has unwound a lot of the tuple mangling. Let’s take a look at the big picture again.

class PathHelper:
    def __init__(self, space, source, target):
        self.space = space
        self.source = source
        self.target = target

    def compute_via(self):
        length, theta = self.get_length_and_theta()
        y_offset = 5
        up = self.rotate_offset(length//3, y_offset, theta)
        dn = self.rotate_offset(2 * length//3, -y_offset, theta)
        return [up, dn]

    def rotate_offset(self, x_offset, y_offset, theta):
        x_rot, y_rot = self.rotate(x_offset, y_offset, theta)
        cell = self.source.at_offset(x_rot, y_rot)
        return cell

    def get_length_and_theta(self) -> tuple[float, float]:
        sx, sy = self.source.xy
        tx, ty = self.target.xy
        dx = tx - sx
        dy = ty - sy
        length = math.sqrt(dx * dx + dy * dy)
        theta = math.atan2(dy, dx)
        return length, theta

    @staticmethod
    def rotate(x, y, angle):
        x_rot = x * math.cos(angle) - y * math.sin(angle)
        y_rot = x * math.sin(angle) + y * math.cos(angle)
        return int(x_rot), int(y_rot)

I don’t think we use space at all. Change Signature to remove it. Commit. And let’s sum up, this is plenty for an afternoon jaunt.

Summary

By means of simple refactoring steps, mostly machine-supported, we have a PathHelper that is notably improved over the original. At no point did our fingers leave our hands, nor did we ever break the tests. Well, test. But it’s a pretty good test.

There are some magic numbers in here, the 1/3, 2/3, and 5. We could make them into some kind of parameters, but at this writing we don’t need any flexibility: we’re just going with a simple offset up and down of 5 cells, at the 1/3 points on the line between source and target.

We are still not checking to see if the points are available. My plan is that we’ll do that in between the up = and dn = in compute_via, and the return, returning a partial or empty via depending on the result we get. I’m not planning to try to recover if one or both of the desired cells is not available.

Bottom line: the code is a bit better. We inch forward again. This is the way.

See you next time!