I’m very confident that our connecting rod is working properly. The code could use a little improvement.

Many Words, Few Lines of Code

A careful reading of my innumerable articles would discover that quite often, in a couple of hours of writing-while-programming, the whole article is about a very few lines of code. I fancy that if I didn’t try to write down my every thought, I might get more done, but in fact my point here is to explore the detailed thinking that goes into our work.

My friend GeePaw Hill tells us that we programmers are properly in the business of changing code, not just barfing it out once and for all and moving on. Of course, these days, some programmers are in the business of making up intricate prompts that will cause an “AI” to produce code that any average programmer would be less proud of than had they written it themselves, and rightly so.

As I am long retired, I program for the joy of creating something that didn’t exist a short while ago, and for the joy of shaping the code into something that suits my aesthetic taste regarding what code should look like. Once the “AIs” write all the code, assuming that society has not fallen by then, perhaps programming will be come a hobby, a craft like knitting or making endless tables, that we share on the internet while the robots do all the work.

Maybe not. Anyway, that’s basically what I’m doing now. And so, quite often, as today, I focus on some small thing. I should mention, however, that I believe that by the nature of code, almost all the work we do is made up of small things. Lots of small things, held together by other small things.

I say “is made up”. I should say “can be and should be made up of small things”. Too much code consists of huge function with many parameters, massive classes with many mutable members, and so on. In my view, that’s not code to be proud of. And if you’re not able to be proud of your work … well, that makes me sad.

And today I want to make the code that we have here something we can be more proud of.

The Linkage Problem

The problem that I’m “solving” has to do with expressing a mechanical linkage, such as a locomotive drive and valve train, and calculating its motion in operation. The ultimate intent is to animate such a linkage. Why? Because I want to.

My vision of the solution is that we’ll have a number of objects representing “linkage components”, such as wheels, cranks, or rods, and that we’ll build a data structure that will let us animate the linkage. We will not be doing real kinematics with all its matrix calculations. Instead, we’ll set the positions of some root components, and then calculate the positions of their downstream components, using simple geometry.

I have sketches, initial versions of a Wheel, a Crank, and a ConRod. My initial implementation of the ConRod did not work correctly. I came to realize that I couldn’t debug it to a state I trusted, so I backed off and worked out the geometry and then test-drove a function to do the calculation of the pushing end of the rod, given about a zillion input arguments. And yesterday, I used that function in the ConRod class, and all the tests run, and the animation looks good. (I’ll probably show you that today, depending on future events. Anyway it works.)

Today, we’re going to look at the code, see why we don’t like it, and make it better. Let’s get started.

What’s Not to Like?

class ConRod:
    def __init__(self, 
        constraint: Wheel, 
        radius, 
        length, 
        lead_angle, 
        piston_angle):
        self._constraint = constraint
        self._radius = radius
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        self._lead_radians = lead_angle * math.pi / 180
        self.line = None

The object ConRod needs a parent constraint: all our objects have at least one of those. A connecting rod has a length, so that’s good. And it is arranged to address a piston rod at some angle to the horizontal, because sometimes the pistons are at an angle. But the lead_angle and radius aren’t about the ConRod at all. They are properties of the Crank, which is attached to the wheel. The ConRod should depend on a Crank, not a Wheel. If it did, it could fetch the radius and lead angle, or perhaps even have help from the Crank.

So our first mission today will be to replace the constraint with a Crank, not a Wheel, and make everything work. We’ll try to go in tiny steps, keeping all the tests working.

Speaking of tests, we have lots of tests for the function that lies within, but not many for the ConRod itself. Since we’ll be changing how ConRod works, I’ll be more comfortable with a few more tests on that object. Best buckle down and do it.

We just have these tests for ConRod itself:

    def test_basic(self):
    def test_90_degrees(self):
    def test_tilt_90(self):
    def test_at_45_offset(self):

I add one more that touches all the bases:

    def test_complicated(self):
        origin = vector(2,3)
        wheel_radians = 19
        piston_angle = 57
        lead_angle = 11
        radius = 1
        length = 5
        wheel = Wheel(constraint=origin, angle_degrees=wheel_radians)
        con_rod = ConRod(
            constraint=wheel, radius=radius,
            length=length, lead_angle=lead_angle, piston_angle=piston_angle)
        con_rod.update()
        start = con_rod.crank_position()
        finish = con_rod.constrained(0)
        self.verify_constraints(
            radius, start, finish, origin, length, piston_angle*math.pi/180)

Green. Commit.

My Cunning Plan

I think what I’ll begin with is this:

  1. Rename the current constraint member to wheel or wheel_constraint;
  2. Change object signature to expect a Crank as constraint.

I was going to have a #3, either to fix up the tests to have a suitably-configured Crank, or to start modifying the code to rely on Crank and let the tests guide me. I’ll probably do the former but let’s do this much first. As soon as I change the signature I’ll have to change the tests anyway.

class ConRod:
    def __init__(self, wheel: Wheel, radius, length, lead_angle, piston_angle):
        self._wheel = wheel
        self._radius = radius
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        self._lead_radians = lead_angle * math.pi / 180
        self.line = None

Green. Commit.

I should mention that PyCharm went through and renamed all the parameters in the tests. Love it!

I decide to require the long form parameters, so I change the __init__:

    def __init__(self, *, wheel: Wheel, radius, length, lead_angle, piston_angle):

The * causes Python to demand the long form definition of parameters wheel=... instead of allowing the parameters without the keywords.

Now let’s call the Crank _constraint. Change Signature.

I’m waffling about the names. Right now I have this:

class ConRod:
    def __init__(self, *, crank, wheel: Wheel, radius, length, lead_angle, piston_angle):
        self._constraint = crank
        self._wheel = wheel
        ...

My thinking to date was to have all the objects refer to “constraint” but that’s so generic, and today I don’t think I like it. We’ll see what we like as we go.

What will the tests do? They won’t compile, because none of them have a crank to supply. Fix them up, like this one:

    def test_complicated(self):
        origin = vector(2,3)
        wheel_radians = 19
        piston_angle = 57
        lead_angle = 11
        radius = 1
        length = 5
        wheel = Wheel(constraint=origin, angle_degrees=wheel_radians)
        crank = Crank(wheel, radius, lead_angle)
        con_rod = ConRod(crank=crank, wheel=wheel, radius=radius, length=length, lead_angle=lead_angle,
                         piston_angle=piston_angle)
        con_rod.update()
        start = con_rod.crank_position()
        finish = con_rod.constrained(0)
        self.verify_constraints(
            radius, start, finish, origin, length, piston_angle*math.pi/180)

The crank = line is new. I think I’d like to make all these objects have keyword parms. We’ll see.

I think this test should run. It does. Make similar changes to the others. Green. Commit: added Crank parameter to ConRod.

OK, I think my tests have configured the crank correctly. So let’s look at the code of ConRod and see how to use its connections correctly. We can start with the init:

class ConRod:
    def __init__(self, *, crank, wheel: Wheel, radius, length, lead_angle, piston_angle):
        self._constraint = crank
        self._wheel = wheel
        self._radius = radius
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        self._lead_radians = lead_angle * math.pi / 180
        self.line = None

We should get the radius from the crank. Oh, and let’s require a Crank type there. And rename the _radius member? Maybe later. One thing at a time. We could fetch the member but I think I want a method for radius().

class ConRod:
    def __init__(self, *, crank: Crank, wheel: Wheel, radius, length, lead_angle, piston_angle):
        self._constraint = crank
        self._wheel = wheel
        self._radius = crank.radius()
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        self._lead_radians = lead_angle * math.pi / 180
        self.line = None

Green. Commit: using crank.radius().

Change Signature to remove radius parm. Green. Commit: remove radius parm.

Lead radians is a crank property. Use that. Green. Commit: fetch lead radians from crank.

Remove the parameter from the init with Change Signature. Green. Commit: remove lead_angle parm.

Here’s init now:

class ConRod:
    def __init__(self, *, crank: Crank, wheel: Wheel, length, piston_angle):
        self._constraint = crank
        self._wheel = wheel
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        self._radius = crank.radius()
        self._lead_radians = crank.lead_radians()
        self.line = None

We can eliminate the wheel parm by fetching from Crank. Let’s do that. I think we could save some steps by taking larger ones but we’ve been doing so nicely with these tiny ones.

        self._wheel = crank.parent()

Green, commit: fetch wheel from crank.

I wonder how many accesses there are to radius and lead radians. One each. Let’s inline them. I think that’ll be a two-step process.

class ConRod:
    def __init__(self, *, crank: Crank, wheel: Wheel, length, piston_angle):
        self._crank = crank
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        self._wheel = self._crank.parent()
        self._radius = self._crank.radius()
        self._lead_radians = self._crank.lead_radians()
        self.line = None

Green. Commit: refer to member _crank_ setting up members.

Find the refs and inline. Green. Commit: inline radius and lead angle.

We’re left with this in init:

class ConRod:
    def __init__(self, *, crank: Crank, wheel: Wheel, length, piston_angle):
        self._crank = crank
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        self._wheel = self._crank.parent()
        self.line = None

Let’s see who’s using the wheel, so that we can refer to the crank instead. Here are two right next to each other:

    def wheel_origin(self):
        return self._wheel.constraint
    def wheel_radians(self):
        return self._wheel.angle_radians

I think we can implement an origin method on Crank. Should it be origin or center? Origin for now:

class ConRod:
    def wheel_origin(self):
        return self._crank.origin()
class Crank:
    def origin(self):
        return self._parent.constraint

Green. Commit: fetch wheel origin via crank.

Same thing with the wheel_radians, I guess. Green. Commit: fetch wheel radians via crank.

What other references to the wheel are there? One in the drawing code, which needs a crank added as well. Works. Green. Commit.

Change Signature to remove wheel. Commit: remove wheel parm from ConRod.

class ConRod:
    def __init__(self, *, crank: Crank, length, piston_angle):
        self._crank = crank
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        self.line = None

And for your edification:

movie

Summary

A dozen commits, mostly after one automated refactoring step. An hour’s work, counting the 300+ of text lines here. A much simpler class interface.

Think about that. Green to green, one tiny step at a time. So nice!

Are we done here? Certainly not. Code, like art, is never finished, only abandoned. Our changes have rippled down through Crank and Wheel, and we have inconsistent naming and use of properties sometimes and methods sometimes. I continue to waffle about that, and about the use of _private member variables. So we’ll continue to improve things.

That said, we might do another component first. That might give us a bit of clarity about the naming.

Either way, see you next time!