One nice thing about gentle refactoring is that often we get ideas for how to make things better. Today …

I’m pretty confident that we can get all our linkage components working such that they only use the start and finish points from their parent. This may not hold true for all the components we ever build, but if not, we can change it again. So but anyway … I had an idea. No, really, it could happen.

Idea
What if instead of allowing our components to grope around in their parent object, we were to pass start and finish to the update method?
Follow-on Idea
What if we were to have update return the object’s own start and finish?

If we were to do those things, and beef up the collection that represents the linkage, we could certainly eliminate the need to grope around, and possibly we could avoid the need for the parent pointer in the components, with our component collection keeping track of the parentage. That might give us simpler code, better isolation of the objects, and quite possibly a smoother shave.

I’ve decided that we’ll try this idea, by which I mean make it work unless there is some fatal flaw that my few minutes’ musing hasn’t identified.

Intuitively, it seems that we should be able to refactor to pass the two points to update, while leaving the update alone, to fetch its own copies of the start and finish, as they do now. Then we can adjust each update method one at a time. keeping the airship flying the whole time. And returning the two points can be deferred, since as things stand, we do have methods to return them.

This is an over-simplification, I’m sure, not least because our objects aren’t yet fully consistent in their use of start and finish. We could, of course, continue as we were, saving this change for later, but I am inclined to go ahead with it now.

Let’s look to see how we might use the PyCharm Change Signature and other refactoring operations to make this job safe and easy. When you specify a Change Signature, you can enter default values for parameters, which PyCharm will insert into the calls. I’m not sure quite how to use that capability, so let’s look at some current update calls.

There’s this one in the code that draws my animations:

for component in components:
    component.update()
    component.init_draw(canvas)

I’ll do that by hand, right now … no, in fact I won’t. Despite my decision above, I think we need to do a bit of preparation before we proceed with this. Patience, Prudence, patience.

We’ll assume uniform access in each component to three things: parent, and start and finish. I’ll drive those in by making the change I want in the loop above and fixing the breakage:

for component in components:
    s = component.parent.start
    f = component.parent.finish
    component.update(s, f)
    component.init_draw(canvas)

I’ve decided that parent, start, and finish will be properties, not methods. I may regret that, but I’m continually working between properties and methods, to build up a better intuition about when to use properties.

PyCharm tells me that update does not accept parameters. I knew that. Since it’s just four classes, I just go through and add in the two parameters in each update method. The tests are probably not happy now. Right, they all complain. So now I’ll just tick through them fixing all the update calls. I had intended to use Change Signature to help me but I got ahead of myself.

After a bit of messing about, my tests all run again, but the drawing code doesn’t draw, because Wheel’s parent is a vector, an egregious hack, and my simple init in the loop shown above doesn’t work.

A bit of hackery and the tests and drawing work. I’ll take a short break: that was tense for a moment there.

OK, I’m back. There was some slightly grotesque hackery needed in the Tk code, because of the wheel parentage:

def animate_linkage(components):
    canvas.scale("all", 0,0, 1/scale, 1/scale)
    for component in components:
        s = f = None
        if type(component) != Wheel:
            s = component.parent.start
            f = component.parent.finish
        component.update(s, f)
        component.draw(canvas)
    canvas.scale("all", 0,0, scale, scale)
    root.after(30, animate_linkage, components)  # Call again after 30ms

For now, I just check for Wheel and pass None as the parameters to update. We’ll sort that out in due time, unless I forget. I’ll write it on a card in red to avoid that.

The tests are green and the animation runs. Commit: passing start and finish into all updates.

That was a massive update, 7 files, and it has been an hour since I started. Not good, but I think we got out OK. Given that we wanted the Tk to work, I’m not sure how to have done it more incrementally, but if I had accepted that the Tk wouldn’t work till the end, there was probably a one-by-one update possible.

Anyway, now I think the next phase is to change the update methods to use the provided start and finish rather than fetching them. We also want to return those values from the update but I think we’d best do that separately.

Let’s do Piston first, it’s pretty simple: it goes like this:

class Piston:
    def update(self, start, finish):
        self._start = self._parent._finish
        finish_offset = vector(self._length,0).rotate_xy(self._radians)
        self._finish = self._start + finish_offset

I reckon we can fix that right up:

    def update(self, _start, finish):
        self._start = finish
        finish_offset = vector(self._length,0).rotate_xy(self._radians)
        self._finish = self._start + finish_offset

Tests and animation run. Commit: Piston no longer references parent. And, you know what, let’s do return the results:

    def update(self, _start, finish):
        self._start = finish
        finish_offset = vector(self._length,0).rotate_xy(self._radians)
        self._finish = self._start + finish_offset
        return self._start, self._finish

Green, of course. Commit: Piston update returns start and finish.

We can add the return to wheel, but we still have that issue with its parent being a vector. Note made.

    def update(self, start, finish):
        self.angle_degrees = self.angle_degrees + 4
        return self.start, self.finish

Just added the last line. Commit: Wheel update returns start finish. Still accessing parent.

An attempt at Crank fails, and I roll back. We’re only part way done with these changes, but we are green and the code is fully committed. I’ll sum up, take a break, maybe run an errand.

Summary

I still feel good about the idea of passing and returning start and finish to and from update. However, the individual components each came along at a slightly different time, according to evolving “standards” of how to write them, and each with their own special needs. So inside the objects, they are differently ready (or unready) for this change. I’d like to come at them with fresh eyes.

I do have to show this code to FGNO tonight. That should be an interesting time, as it is certainly not Code To Be Proud Of at the moment.

See you next time, with a report.