Yesterday’s closing idea looks good to me. Let me know what you think, if you care to. We find that we’re not quite finished. LLM-AI diversion at end.

Yesterday, we left the code like this, with that long comprehension that even includes a walrus operator := for extra complexity:

    def compute_waypoints(self, segments=3, offset=5):
        step = self.length//segments
        return [rotated
                for i in range(1, segments)
                if (rotated := self.cell_at_offset(i, step, offset)) in self.map]

    def cell_at_offset(self, i, x_offset, y_offset) -> Cell:
        up_dn = 1 if i%2 == 1 else -1
        x_rot, y_rot = self.rotate(i*x_offset, up_dn*y_offset, self.theta)
        return self.source.at_offset(x_rot, y_rot)

I decided to leave that in and closed with an idea for improving it, saying:

In fact, having written that, I already have a better idea. We won’t explore it, but I think we could get down to something like this:

    def compute_waypoints(self, segments=3, offset=5):
        ...
        return [cell
                for cell in offset_cells(...)
                if cell in self.map]

That would apparently loop twice, but do we care? Not if offset_cells was a generator …


In the afternoon, I had a chance to play a bit and here’s what we have now.

    def compute_waypoints(self, segments=3, y_offset=5):
        x_step = self.length//segments
        return list(self.rotated_cells(segments, x_step, y_offset))

    def rotated_cells(self, segments, x_offset, y_offset):
        for i in range(1, segments):
            cell = self.cell_at_offset(i, x_offset, y_offset)
            if cell in self.map:
                yield cell

    def cell_at_offset(self, i, x_offset, y_offset) -> Cell:
        up_dn = -1 if i%2 == 0 else 1
        x_rot, y_rot = self.rotate(i*x_offset, up_dn*y_offset, self.theta)
        return self.source.at_offset(x_rot, y_rot)

rotated_cells generates the cells that we like, by simply yielding a cell only if it is in the map. That lets compute_waypoints convert the generator to a list, because we need a list of waypoints. At least I think we do. Yes:

    def find_path_via_waypoints(self, t, rooms, waypoints):
        points = [self] + waypoints + [t]
        cells = []
        for p1, p2 in zip(points, points[1:]):
            cells += p1.find_path(p2, rooms)
        return list(set(cells))

We could convert to a list there but it’s better for a method to return what people want, not part of what they want.

Aside
I’d like to avoid that summing loop in find_path_via_waypoints but since find_path returns a list it’s not as simple as it might be. We could create a list of lists and then flatten it.

We’re sitting on a save point, let’s see what we can do.

    def find_path_via_waypoints(self, t, rooms, waypoints):
        points = [self] + waypoints + [t]
        cell_lists = [p1.find_path(p2, rooms) for p1, p2 in zip(points, points[1:])]
        cells = [cell for cell_list in cell_lists for cell in cell_list]
        return list(set(cells))

Yeah, no. Belay that. I don’t like that flattening trick at all. This is not as clear as the original. Roll back after an amusing attempt.

Anyway, we now have an improved bit of code in compute_waypoints and its called methods, better than yesterday and yesterday’s version was acceptable.

Let’s compare what we had two days ago to what we have now, see what we think:

Two days ago

class PathHelper:
    def __init__(self, source, target):
        self.source = source
        self.target = target
        rooms = [source.room, target.room]
        self.map = source.path_map(lambda c: c.is_available or c.room in rooms)

    def compute_waypoints(self, segments=3, offset=5):
        length, theta = self.get_length_and_theta()
        result = []
        for i in range(1, segments):
            rotated = self.rotate_offset(i * length // segments, offset, theta)
            if rotated in self.map:
                result.append(rotated)
            offset = -offset
        return result

    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

    @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)

    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

class PathHelper:
    def __init__(self, source, target):
        self.source = source
        self.length, self.theta = self.get_length_and_theta(source, target)
        rooms = [source.room, target.room]
        self.map = source.path_map(lambda c: c.is_available or c.room in rooms)

    def compute_waypoints(self, segments=3, y_offset=5):
        x_step = self.length//segments
        return list(self.rotated_cells(segments, x_step, y_offset))

    def rotated_cells(self, segments, x_offset, y_offset):
        for i in range(1, segments):
            cell = self.cell_at_offset(i, x_offset, y_offset)
            if cell in self.map:
                yield cell

    def cell_at_offset(self, i, x_offset, y_offset) -> Cell:
        up_dn = -1 if i%2 == 0 else 1
        x_rot, y_rot = self.rotate(i*x_offset, up_dn*y_offset, self.theta)
        return self.source.at_offset(x_rot, y_rot)

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

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

The new version is one line longer than the old. I think it would be fair to say that it adds complexity by using a generator with yield, although that capability is so central to Python that we should be familiar with it. The up_dn trick is a bit obscure. Would naming it sign help? I’ll try that and a couple of other renames:

    def cell_at_offset(self, step_number, step_size, y_offset) -> Cell:
        sign = -1 if step_number % 2 == 0 else 1
        x_rot, y_rot = self.rotate(step_number * step_size, sign * y_offset, self.theta)
        return self.source.at_offset(x_rot, y_rot)

Yes, I think that’s preferable. Commit: tidying.

As I review this now, I think a pass could be made making the names of the parameters a bit better, a bit more consistent. We’ll let it ride for now.

What this code really lacks is that it doesn’t make clear what it is doing, nor why. We could add a comment, but I was taught that a comment is the code’s way of asking to be made more clear.

What is this class? What is its purpose? What does it do? It’s not just a “helper”. It takes a pair of cells, between which we’re about to draw a more or less straight path, and it returns a collection of cells, starting with the first one provided, ending with the second, and with a requested number of intervening points in between, which wander a prescribed distance away from the nominal path.

Is it a WanderingPathMaker? A SegmentedPathMaker?

Maybe the responsibilities aren’t divided up quite right. We use the PathHelper like this:

main.py
    ...
    helper = PathHelper(start, target)
    waypoints = helper.compute_waypoints(5, 10)
    path = start.find_path_via_waypoints(target, [s, t], waypoints)
    ...

Ah! That’s not really great, is it? We should be able to say something like

path = start.path_to(target, segments=5, wandering_distance=10)

I’m glad we had this little chat. What we’re seeing here is that we aren’t really finished solving the pathing problem, so the objects aren’t quite right.

What we’ll do next time is try to make them more right, and we’ll sum up now. There is a lesson here and it’s a tricky one.

Summary

There is a serious lesson here.

The working code above, in main, isn’t far off from plenty of production code that I’ve seen, even production code that I’ve written. The thing is, it isn’t very good.

The programmer writing main has been asked to make the paths between sources and targets wander more than they generally do if we just draw the path using our nearest point logic. So we give that programmer a handy method, find_path_via_waypoints, which they can use to fill in any wandering they want, by providing waypoints.

And then they’re not satisfied, because they don’t know how to create decent waypoints and don’t even have reasonable access to some of the information they’d need, so we write a PathHelper object that they can use and we say:

OK, here you go, just create a PathHelper, ask it for whatever waypoints you want, and then use find_path_via_waypoints and we walk away. And it works, so the programmer does it that way.

They do it that way every time they want a wandering path, which is often. The code gets a little bit worse every time. Maybe they even write a convenience method of their own and tuck it away somewhere in their classes, because they don’t dare change Cell, or don’t feel confident enough, or feel they haven’t the time to do it.

I predict that tomorrow, or some day real soon now, we’ll improve the situation substantially, providing a method like the path_to sketched above. That’s easy: we could do it right now.

But then we’ll look at the situation with the PathHelper and Cell relationship and see whether we can improve the code’s expression of what and why. I predict that we’ll make it better.

In addition, we’ll wind up with the PathHelper, if it even still exists, as a class that no one has to know about other than Cell. In that context, even if we can’t get the names quite perfect, it’ll be more clear, and fewer programmers ever have to look at it anyway.

When we review real live production code, we often see this kind of situation, where instead of the available classes and methods being just right for what we need to do, we get some kind of bizarre kit of objects and methods that can be arranged, like a house of cards, to do almost what we want in a way we can almost understand.

That kind of code is not finished.

Unfortunately, of course, real world programmers are under pressure to do the next thing as soon as the current thing seems to work. And that’s why legacy systems become harder and harder to work with. If I were ever again condemned to work in such a situation, I’d take every opportunity to make the code around me a bit better, because if we use it or tweak it once, we’ll probably be back again, and we can make the next changes easier, or at least no harder.

The situation with LLM-AIs is even worse.

If you are condemned to work with an LLM-AI (ptui!), my heart goes out to you. The demon—it’s not a helpful genie, it’s a chaotic demon—will produce code like our three-liner above, except it’ll be ten or thirty lines. And your job will be to see if it works and approve it. And you will approve it, because the checking job is far less interesting than the creative job, and it’s tedious, and anyway the code looks OK.

And the LLM-AI-created system will be worse than a human creation every time. The programmer’s life will be made worse. The code will be made worse. The world will be made worse.

I have no advice about this. Maybe we should all get out while we can. Maybe programmers should unionize. Maybe we’re just screwed.

But we’ll have some fun here while it lasts.

See you next time!