Starting late today, might end early. Let’s see if there are right-sized things to do. Good thinking, but the work does not go as I anticipated.

Day before yesterday, we extracted a class from ConRod, ConRodPainter, to handle the job of drawing the ConRod’s representation on our demonstration screen, used primarily as a visual assurance that nothing has gone wrong. The benefit of this extraction is that it separates the calculation part of a component from the ancillary drawing of it.

Idea
It occurs to me as I write this that one possible use of this program is to produce a series of “snapshots” of the positions of the linkage, to be used in a screen or 3D animation sequence. If we do that, the snapshot logic will stand roughly where the drawing logic does, expecting to be called frequently with various values, and doing something, presumably writing out a file rather than drawing on the screen.

So there is a possibility that there will be more than one kind of ancillary activity. It seems likely that we could handle that new idea similarly to the handling of drawing. That suggests that we should consider generalizing the drawing further, with some kind of dependency injection or something similarly fancy.

When I arrived at the first line of this article, “Day before yesterday”, I was planning to discuss whether and when to create Painter objects for the other components, Wheel, Crank and so on. Now we have an even more grand question before us: whether or when to put in a more general dependency scheme to handle the obviously many and varied things we’ll surely want to do someday.

Rule of Thumb

My general rule of thumb for refactoring is not to refactor an existing object until that object needs work for some other more value-focused reason. Until we have to open that file and work on it, it is doing no harm, working as it is. For some reason, mostly the fact that drawing doubled the size of ConRod, we elected to improve its code. We weren’t actually working on it at the time: it just came up and we decided to work on it. We could consider that work to be an experiment, or a random code improvement on a day when we weren’t up to anything really hard.

But we have no burning need to improve the other classes: they are working just fine. So, by my lights, we do best to let them be until we are working on them out of some other necessity. We have done our new Painter code in a form that can be copied, and that might be canonized at some future time, but we made no changes that force any other object to change. So let’s not.

As for the notion of providing a way to have more than one kind of related activity, dumping information to a file being the only example we’ve thought of? We have no call whatsoever to do that. So let’s not.

At this point, some team member will say “But if we do it now, it will be easier and cheaper than if we wait until there are lots more components to be refactored, so we should do it now to save time and money”. And I will say:

Let’s not, for now, and here’s why.

We only barely have one dependency thing, the drawing of one component. We may or may not ever do another. The only one we’ve thought of is a file dumper, and we don’t even know whether we’ll have to do that. Even if we do, we don’t know now what it has to do, so any work to support a general dependency model will be speculative. Generally, when I do it, my speculation is partly right and seriously wrong. So let’s not, for now.

So, by my lights, we shouldn’t add Painters to our other components right now, and we certainly shouldn’t set up for multiple dependencies. If we knew what was coming, and we felt that it would be tricky or time-consuming, I might advise a low-effort experiment or two, “spikes”, to learn a bit about how we might solve the problem. As things stand, I think we’re pretty ready for whatever may come down the line.

What Shall We Do?

One thing I always recommend (to myself, no one else really listens) is to look at recently committed code a day or two later, with somewhat fresh eyes, to see if there are any rough edges that should be cleaned up. A conscious choice to do that seems better to me than walking away and then when we come back for some other reason, tripping over a loose brick that we could have spotted. Of course, under pressure, this kind of check will likely be skipped, and down that path lies a bunch of somewhat messy code, getting messier.

I don’t think there is a standard rule or percentage for deciding this. In my own work, checking yesterday’s code today seems to make sense, but I am looking for something to think and write about, not trying to get something done. But how much of a leftover mess is ideal in your high-pressure feature-oriented sweatshop? You get to decide, but the chef sharpens his knives often, because sharp knives make him go faster.

Our other main possible work item is the ArcConRod, the connecting rod object that connects to an arc rather than a straight line, such as a rod operating a bell crank to change the direction or stroke of the originating rod. We have a start at that. It’s several days old, so I will have to review it to know what it needs, but I think we just have a test or two working the simplest math case out longhand, so we need to isolate the general math solution. I’m assuming will follow the transform-oriented scheme of ConRod, where we reduce the problem to a simple zero-based form, solve that, and transform the answer back to the more general location and rotation. In any case, we’ll want to review what we have and make a plan.

So let’s do both. First, a quick review of the ConRodPainter, and then a look at ArcConRod.

class ConRodPainter:
    def __init__(self, piston_radians):
        self._piston_radians = piston_radians
        self._cached_line = None

    def init_draw(self, canvas, start, finish, wheel_center):
        length = finish.distance(start)
        self._init_rod_picture(canvas, start, finish)
        self._init_rod_background(canvas, length, wheel_center)

    def draw(self, canvas, start, finish):
        canvas.coords(self._cached_line,
                      start.x, start.y,
                      finish.x, finish.y)

    def _init_rod_picture(self, canvas: Canvas, start, finish):
        self._cached_line = canvas.create_line(
            start.x, start.y,
            finish.x, finish.y,
            width=3, fill="red")

    def _init_rod_background(self, canvas, length, wheel_center):
        point_on_rod_line = vector(2 * length, 0, 0).rotate_xy(self._piston_radians)
        self._init_guide_line(canvas, wheel_center, point_on_rod_line)
        self._init_minimum_travel_point(canvas, wheel_center, point_on_rod_line)

    def _init_guide_line(self, canvas, wheel_center, point_on_rod_line):
        canvas.create_line(
            wheel_center.x, 
            wheel_center.y, 
            wheel_center.x + point_on_rod_line.x,
            wheel_center.y + point_on_rod_line.y, fill="blue")

    def _init_minimum_travel_point(self, canvas, wheel_center, point_on_rod_line):
        one = vector(1, 0).rotate_xy(self._piston_radians)
        half = wheel_center + one + point_on_rod_line * 0.5
        s = 0.1
        canvas.create_oval(half.x - s, half.y - s, half.x + s, half.y + s, fill='red')

What I notice about this code is a couple of magic numbers. The 2 that goes into point_on_rod_line, and the 1 that goes into one. The former is an arbitrary length, but I suspect that the 1 actually has to match the radius of the crank in order for the little dot to show up where it should.

I’ll tweak my example code to see if that’s true. Sure enough:

big radius

I changed the radius to 2, which gives the rod a much longer path. And the dot no longer shows the end of the rod’s path. I guess we should see about fixing that. To fix it, we need to pass the radius into the init_draw or the draw. Which means we have to cache it, along with the wheel center that the draw wants.

Or we could ditch the dot. But I think it’s a good check. So let’s change ConRod:

    def update(self, start: VtVector, finish: VtVector):
        self._cached_wheel_center = start
        self._cached_wheel_radius = self.crank_radius(start, finish)
        self._start, self._finish = (
            con_rod_ends_complete(
                piston_radians=self._piston_radians,
                wheel_center=start,
                radius=self._cached_wheel_radius,
                crank_radians=self.crank_radians(start, finish),
                length=self._length))
        return self._start, self._finish

And change signature: …

Belay That!

After a bit of work, getting the necessary value down into the right function, I discover that the point doesn’t go where it should, using the formula we have. My ad-hoc geometry to draw the guide line and maximum rod travel dot appears to be wrong. This change is more than I can do offhand. Reset.

And, whenever we crash and burn, it’s time for a break. Just as well, a late start has me out of phase with the sun.

Summary

I was up for a quick code review and maybe a few name changes sort of thing. I spotted a problem, but didn’t switch modes from simple refactoring to really figuring out what the issue was. A quick change of the literal 1 to 2 would have shown me that I didn’t quite understand the code, but no, I went ahead and assumed that it would work. It didn’t.

So that was easily remedied with a Git Reset Hard, but it also took the wind out of my sails, and I came down off the foils and was wallowing. Fine. I’ll come back when fresh, and either fix the damn dot, or move on.

Live and, if not learn, find out. See you next time!