With four components in place, let’s look at the code and see how it can be improved.

What’s Not to Like?

We have four components, Wheel, Crank, ConRod, and Piston. Piston is not yet moved to the source tree, still residing in the test tree.

Since those classes are all linkage components, one would expect that they would have common elements, a common superclass or abstract type, to which they all adhere. This is not the case.

I think that some of the classes use keyword parameters in their constructor, and others do not. I believe that we should use them throughout, or nowhere. Probably throughout.

Some additional issues include inconsistency in naming the members and methods of the classes, and inconsistent use of properties vs methods. There is at least some code referring to member variables in other classes with underbar names, which conventionally means they are private.

Surely we’ll see other things as we look at the code.

Let’s get to it. we’ll begin by moving the Piston class to its own file in the source tree. PyCharm will do this for us … and it does. I note while doing that that two test files have wound up in the source tree. We’ll test, commit, move those.

That’s just a matter of moments. Now let’s look at the __init__ methods of our working classes, to get a sense of how similar—and how different—they are.

class Wheel:
    def __init__(self, constraint, angle_degrees=0):
        self.line = None
        self.circle = None
        self.constraint = constraint
        self._angle_radians = angle_degrees * math.pi / 180.0

class Crank:
    def __init__(self, parent, radius, leadDegrees=0):
        self._parent = parent
        self._radius = radius
        self._leadRadians = leadDegrees*math.pi/180.0

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

class Piston:
    def __init__(self, *, parent, length, angle):
        self.line = None
        self._start = None
        self._finish = None
        self._parent = parent
        self._length = length
        self._radians = angle*math.pi/180

Note that Wheel, our earliest class, does not flag its members as private, and the other classes do. We’d like to be consistent. We’ll also see, when we get there, that we have inconsistent treatment around what a child component can fetch from its parent, and how it does so.

Note also that ConRod and Piston use keyword arguments and Wheel and Crank do not. The clue is the * as the first parameter after self. This means that when we create the class we must provide not just the value of each parameter, but its name as well, like this:

con_rod = ConRod(crank=crank, length=length, piston_angle=tilt_angle)

I like that notation and am of a mind to standardize on it, especially since each linkage component seems to need slightly different specs.

Recall that I said we’d see other issues? Here’s one: the member line in three of these classes is a mutable Tk element, saved so that the drawing code can update it.

It is generally considered to be bad form for an object to know how to render itself, although I confess that I can think of no object that should know better. We should at least nod to this received wisdom and consider breaking out the drawing of the components from their computation: it is certainly clear enough that drawing and computation are two different things, so having them in different classes does make some sense.

But wait, there’s more!

The Wheel has a member constraint, which is supposed to return the wheel’s center. At the time I wrote Wheel, I was thinking in terms of constraints for each object. The Crank class uses parent for that purpose, as does Piston, but ConRod uses crank.

I think we should standardize on the parameter name parent, and allow the class to name its corresponding member variable whatever makes sense to the object. Each component refers to its parent object to get certain parameters that it needs—and those parameters are different. The Wheel expects a vector, its origin. The Piston just wants to know the coordinates of the pushing end of its parent (typically a ConRod), also a vector. But the ConRod needs the center of its parent (a Crank) and the radius of the Crank.

Aside
I just noticed a naming issue in Crank. It has a method wheel_origin, which returns self._crank.origin(). It should be called crank_origin or crank_center, as the ConRod should not know about the Wheel, its grandparent. I think that is a leftover from an early implementation of the ConRod, which was constructed with information that we subsequently derived from its parent.

While I’m noticing, I’ll rename that method to crank_origin for now. We’ll think about center vs origin later. We could also consider crank_position, I suppose. We need to settle on a set of names and stick with them … or change all of them at once, to make browsing this code more smooth.

For now: crank_origin is the name. Changed, tested, committed. I noticed while doing that that there is also a method wheel_radians, which does in fact refer to the current rotation of the wheel, which is an input into the connecting rod geometry solution. Possibly we should be asking the Crank for a composite angle: I’m not sure whether the ConRod algorithm needs all the angles broken out or not. We’ll make a card for these, since we’ve noticed and are not chasing them just now.

Back to the main thread, let’s require keyword parameters on all the classes. This may be a bit messy: I’m not sure whether PyCharm can help with this. I don’t think it can do the job on its own. Let’s see if we can Change Signature and add a * parameter and see what PyCharm says and does. First, Wheel.

Wow! It warned me that there needed to be named parameters after the *, because it gets added at the bottom of the list. When I moved it up to the top, PyCharm went ahead and seems to have plugged in the names on all my Wheel constructors. Tests all run. Commit: change Wheel to keyword parameters.

Repeat with Crank. Test; commit. I continue to be impressed with PyCharm.

Now I think I”ll change any parameter that are not parent and should be. Again PyCharm does the work, including changing an accessor in one of the classes. Commit: all components expect parent parameter.

I wind up looking at this class and init:

class Piston:
    def __init__(self, *, parent, length, angle):
        self.line = None
        self._start = None
        self._finish = None
        self._parent = parent
        self._length = length
        self._radians = angle*math.pi/180

There are three mutable members there, line, _start, and _finish. The update command caches _start and _finish, and init_draw and draw, not shown, use them. init_draw creates line and draw updates it.

My overall plan for these objects is that you build a tree of them, keeping a list from root upward, and you do the animation by updating the root (typically a Wheel, whose rotation is incremented a bit), and calling update in sequence from root onward in the tree, until everyone is updated. My thought was that they’d all cache their answers, primarily for efficiency but also to ensure that the update happens only when we really want it to, not every time we draw things or the like. Mostly an efficiency idea.

It’s also more convenient for a given component to ask its parent for the values it wants as if they are “just there”.

I’m rationalizing. It’s that way because I wrote it that way. It seemed reasonable at the time, and doesn’t seem wrong even now, except that it makes all the components mutable.

My friends and colleagues dislike mutable objects. If I can get some time to show this code at FGNO, that’s one of the things I’ll ask that we talk about. I’d like to get some strong views on the table, and my own are more laissez-faire.

I do see the case for some kind of separation here. If we look at the Piston above, we see that three of its members do not change after creation, and three change on every update or draw. (Two on update, one on draw? Interesting …) It does make sense to me that an object should either be all mutable or all immutable, not half of one six a dozen of the other. Maybe there is some kind of result object that the immutable object creates and returns? Worth thinking about.

For now, I’m thinking about breakfast and Sunday morning TV. There’s not much different in the code, just an added * parameter in the inits, and, some PyCharm-provided more explicit constructors, like this:

wheel = Wheel(parent=center, angle_degrees=angle_degrees)
crank = Crank(parent=wheel, radius=radius, lead_degrees=lead_angle)
con_rod = ConRod(parent=crank, length=length, piston_angle=tilt_angle)

But there’s a significant improvement in the code due to the named parameters. A bit harder to type, but PyCharm prompts nicely, and far easier to understand in the reading.

A nice little bit of relaxing code improvement. Great way to start the day.

See you next time!