I'm Not ALWAYS Right ...

Why, only yesterday, I thought I had made a mistake. Turns out I was wrong, I hadn’t made a mistake after all!

Yesterday, in a frame of mind to do a little tidying in ConRodPainter, I ran across a constant value that needed to be variable:

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

That 1 in the vector at the top needs to be the radius of the crank driving the ConRod. I decided to fix it, and went through the rigmarole to pass in the necessary value and use it. Then I ran the picture … and decided that it was wrong. So I reset and ended the session.

Later that day, I came to suspect that I had misinterpreted the picture and that the code was probably correct. So this morning, I patched in a 2 and used a radius of 2, and the picture is just what it should be, with the ConRod just barely reaching the dot drawn by the method above.

So I was wrong when I thought what I had done yesterday was wrong but what I had done in the days before was also wrong. The title of this article is I’m Not ALWAYS Right but sometimes I wonder if it should be I’m Rarely Right. Maybe I could go with I Am Sometimes Eventually Right.

OK, enough of the self-deprecating humor. Let’s do at least two things this morning. Let’s fix this problem, better than we fixed it last time, and let’s think about whether and how we could have avoided the error to begin with.

First the fix.

The ConRod already caches one value for its Painter:

class ConRod:
    def __init__(self, *, length: float, piston_angle: float):
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        # mutable graphic support temps
        self._painter = ConRodPainter(self._piston_radians)
        self._cached_wheel_center = None

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

Let’s cache the radius as well. Extract Variable in the update:

    def update(self, start: VtVector, finish: VtVector):
        self._cached_wheel_center = start
        self._cached_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_radius,
                crank_radians=self.crank_radians(start, finish),
                length=self._length))
        return self._start, self._finish

PyCharm squiggles the new assignment, telling me that I’m defining an instance attribute outside __init__, which is a bad practice and can often reflect a typo. It offers to add the attribute for me. It does, but adds it at the top of __init__. I have to manually move it where I want it like some kind of barbarian:

    def __init__(self, *, length: float, piston_angle: float):
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        # mutable graphic support temps
        self._painter = ConRodPainter(self._piston_radians)
        self._cached_wheel_center = None
        self._cached_radius = None

Now we need to pass the info to the Painter, presumably wherever it is that we pass in the other cached value. Command B finds this:L

    def init_draw(self,
          canvas: Canvas, start: VtVector, finish: VtVector):
        self._painter.init_draw(
            canvas, start, finish, self._cached_wheel_center)

That’s where it has to go. However, I remember a thing from yesterday, which was that when I did the Change Signature refactoring, it mistakenly refactored this code in the drawing main:

..,
side_rod = SideRod(length=7)
components.add(side_rod, crank)
components.apply(lambda c, s, f: c.init_draw(canvas, s, f))

I’ll do the additional parameter by hand. That seems safer than running a machine refactoring that doesn’t know when to stop. We might also change the name of the Painter method not to be the same as the Component method. Too tricky for now. Just pass in the parameter and fix up the Painter:

class ConRod:
    def init_draw(self,
          canvas: Canvas, start: VtVector, finish: VtVector):
        self._painter.init_draw(
            canvas, 
            start, finish, 
            self._cached_wheel_center, self._cached_radius)

And the receiver:

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

We need that information in the background method, so pass it down:

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

PyCharm offers to fix up _init_rod_background and I accept:

    def _init_rod_background(self, canvas, length, wheel_center, radius):
        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)

We need this information in the _init_minimum_travel_point. Add it and let PyCharm help.

    def _init_rod_background(self, canvas, length, wheel_center, radius):
        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, radius, point_on_rod_line)

    def _init_minimum_travel_point(self, canvas, wheel_center, radius, 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')

Now use the value, and rename some stuff:

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

And that works.

The con rod just touches the red dot on full extension. That’s what I wanted … and what I had yesterday. Today’s version is better, I think.

Test. Commit fix issue causing ConRod limit point to be incorrectly displayed.

Now I promised to think about whether and how we might have avoided the original mistake of putting a constant in there where a variable clearly needed to go.

One issue is that I don’t have a lot of good ideas about how to test what is essentially a display function. The relevant code is just that one line:

crank_offset = vector(radius, 0).rotate_xy(self._piston_radians)

Interruption

Hey! Notice that we are accessing a saved value for _piston_radians and a parameter to the method for radius. It seems we should either save all the needed values, or pass them all as parameters, not half of one and six a dozen of the other.

We won’t address that right now, but I’d like to underline that every time we look at the code and think about it, there is a good chance that we’ll see something that should be improved. And we don’t always fix the thing. Sometimes we just decide to let it be, sometimes we make a note to come back later. We do not put a TODO in the code, because our file storage quickly fills up with TODOTODOTOD.

As I was saying, to test that line it seems that I’d have to break it out as a separate method and write a test for it. At the time I wrote it, I knew that radius was 1 in the code that draws the example. I don’t recall thinking “oh that should be a parameter”, but whether I did or not, my judgment at the moment was that we wouldn’t go after another parameter. My practice with graphical stuff is that I kind of hammer at it until it looks right: the geometry code is easy to write and hard to test.

That’s not an excuse: it’s an explanation, a description of how my brain does things.

I used small integer values like one, because I was working the problem on paper and trying to keep the picture and the math simple. Had I not been working with a radius of 1 in the drawing code, the value in the offset line would have been weird. Possibly I tend to accept 1 as a magic number where I wouldn’t accept a 3.45. That’s not clear, because in that same method there is a very magical bit:

half = wheel_center + crank_offset + point_on_rod_line * 0.5

That 0.5 is seriously weird. Do you know what that is? It happens that point_on_rod_line is a point twice the rod length out on the line, just to make the guide line a nice length. And then I divide that by two to get the actual rod length, which we need to decide where the dot should go. And the name, half? That should be point_of_furthest_extension or something like that.

Even though the drawing code is just there for my own visual checking, what we’re seeing here is that it’s pretty messy. The drawing is part of my tool kit: it is a tool that I use in the making of the actual program. In this case, the particular tool, the ConRodPainter, is not as sharp as it could be.

When we’re developing a system, especially one that is going to take a while to build and that will need maintenance over time, we need to pay attention, not just to the app itself, but also to the tools we use to build it.

GeePaw Hill describes the “Shipping App”, the product we’re building for our users, and the “Making App”, the code we develop to help ourselves develop the Shipping App. See this article for more.

The drawing bits are part of my “Making App” and what I’m learning today is that they could use a bit more attention. I need to sharpen my tools a bit.

Conclusion

Nice! I didn’t expect to discover or learn much today. But there it is: pay more attention to the tools.

See you next time!