I’m psyched to add another linkage component, but there are some issues in the code that could use improvement. What should we do?

If I had a manager, which praise all that can be praised I have not, they’d probably be pushing me for features features features. (You might ask yourself why, if I’m going to imagine a manager, I wouldn’t imagine a good one. Might as well ask yourself, because I sure don’t know.) And, pliant and tender young thing that I am, I might go ahead and layer in another component, following the current code scheme. Since the current code scheme, how can I best put this, could use a little improvement, the result would be a feature, which would probably work, living in code that needed somewhat more than a little improvement.

I do not have a manager, see above re: praise, and I’m going to at least consider what the code needs before perpetuating whatever mechanisms it presently involves. In particular, I am concerned about the need to insert a minus sign to get the ConRod to go around the wheel the same direction that the wheel itself was going. Let’s start there.

Here’s the whole ConRod class. I expose it all to you because we need to consider the whole thing, not just a line or two, if we are to make decent decisions:

class ConRod:
    def __init__(self, constraint, radius, length, tilt_angle):
        self._constraint = constraint
        self._radius = radius
        self._length = length
        self._tilt_radians = tilt_angle * math.pi / 180

    def constrained(self, number):
        wheel_angle = self._constraint.angle_radians - self._tilt_radians
        crank_x = self._radius * math.cos(wheel_angle)
        crank_y = self._radius * math.sin(wheel_angle)
        self.crank = VtVector((crank_x, -crank_y, 0))
        x_proj = math.sqrt(self._length * self._length - crank_y * crank_y)
        constrained_x = crank_x + x_proj
        un_rotated = Vector((constrained_x, 0, 0))
        rotated = un_rotated.rotate_xy(self._tilt_radians)
        center = self._constraint.constrained(0) # constrained?
        return rotated + center

    def init_draw(self, canvas):
        target = self.constrained(0)
        crank = self.crank + self._constraint.constrained(0)
        self.line = canvas.create_line(
            crank.x, crank.y,
            target.x, target.y,
            width=3, fill="red")

    def draw(self, canvas):
        target = self.constrained(0)
        crank = self.crank + self._constraint.constrained(0)
        canvas.coords(self.line,
                      crank.x, crank.y,
                      target.x, target.y)

    def update(self):
        pass

The __init__ seems reasonable at first glance. When we look at constrained, we know that that method’s assigned duty is to compute and return the resultant, output, end point of the ConRod, as opposed to the end that is um constrained by the _constraint object. In our case, that is a Wheel.

There is a small red flag, maybe just a yellow card in there. The method is setting self.crank, a member variable, and that member is not declared in the init. That’s often a signal of something, ranging from a bit of carelessness to an outright error. In our case here, it is saving some information that we use twice(!) below, in init_draw and draw.

That draws our eye to the duplication that’s apparent in those two methods. Duplication is a clear sign that there’s improvement to be had, and if we leave it in and discover the need to change one of those methods, it is likely that the other one will need changing as well. And, those of us who are human have some chance of failing to notice that requirement, introducing a defect.

There’s another issue with caching that crank value: the other member variables are invariant over the life of the object. This one changes during use. We can certainly tolerate that if we have to, or we can remove the concern somehow. What possibilities are there?

  1. We could just compute it over and over again, at some small cost in time but arguably an improvement in overall design.
  2. We might be able to come up with a helper object that is mutable, leaving our ConRod immutable.
  3. And, of course, we could just live with it.

We look at how crank is computed:

wheel_angle = self._constraint.angle_radians - self._tilt_radians
crank_x = self._radius * math.cos(wheel_angle)
crank_y = self._radius * math.sin(wheel_angle)
self.crank = VtVector((crank_x, -crank_y, 0))

This code seems to me to be very interested in the details of the constraint. It is trying to compute the position of its driven end vis-a-vis the wheel. Let’s ask the wheel for that information instead.

Should I add a test to the Wheel for this new method? If I ever ask myself that question, the answer is “yes”, although existing tests will surely break if I do it wrong. Additionally, I remain concerned about that minus sign in the vector creation.

Let’s try some refactoring, see if we can ease up on this. Extract Variable:

def constrained(self, number):
    wheel_angle = self._constraint.angle_radians - self._tilt_radians
    crank_x = self._radius * math.cos(wheel_angle)
    crank_y = self._radius * math.sin(wheel_angle)
    crank_pos = VtVector((crank_x, -crank_y, 0))
    self.crank = crank_pos

Extract Method:

def constrained(self, number):
    crank_pos, crank_x, crank_y = self.crank_position()
    self.crank = crank_pos
    x_proj = math.sqrt(self._length * self._length - crank_y * crank_y)
    constrained_x = crank_x + x_proj
    un_rotated = Vector((constrained_x, 0, 0))
    rotated = un_rotated.rotate_xy(self._tilt_radians)
    center = self._constraint.constrained(0) # constrained?
    return rotated + center

def crank_position(self):
    wheel_angle = self._constraint.angle_radians - self._tilt_radians
    crank_x = self._radius * math.cos(wheel_angle)
    crank_y = self._radius * math.sin(wheel_angle)
    crank_pos = VtVector((crank_x, -crank_y, 0))
    return crank_pos, crank_x, crank_y

Oh I don’t like that. I didn’t read carefully enough to notice all the uses of those temps. Revert. We’ll do the method on Wheel, with a test. See me learning my lesson again?

    def test_wheel_crank(self):
        loc = Vector((2, 3, 0))
        wheel = Wheel(constraint=loc, angle_degrees=45)
        crank_offset = wheel.crank_offset(radius=1, tilt=0)
        assert crank_offset.x == 0
        assert crank_offset.y == 0

I really expect 0.707 and change but I want to get the method in place, so I’ll accept the fail.

    def crank_offset(self, radius, tilt_degrees):
        effective_angle = self._angle_radians + tilt_degrees*math.pi/180.0
        return VtVector((radius, 0, 0)).rotate_xy(effective_angle)

Note that I renamed the parameter to tilt_degrees, which is the convention I’m trying here, because my users think in degrees and computers like radians.

Test fails as expected:

Expected :0
Actual   :0.7071067811865476

Change the test:

    def test_wheel_crank(self):
        loc = Vector((2, 3, 0))
        wheel = Wheel(constraint=loc, angle_degrees=45)
        crank_offset = wheel.crank_offset(radius=1, tilt_degrees=0)
        sqrt2by2 = math.sqrt(2)/2
        assert crank_offset.x == pytest.approx(sqrt2by2, 0.0001)
        assert crank_offset.y == pytest.approx(sqrt2by2, 0.0001)

I think I want crank_position as well, extend the test and code:

def test_wheel_crank(self):
    loc = Vector((2, 3, 0))
    wheel = Wheel(constraint=loc, angle_degrees=45)
    crank_offset = wheel.crank_offset(radius=1, tilt_degrees=0)
    sqrt2by2 = math.sqrt(2)/2
    assert crank_offset.x == pytest.approx(sqrt2by2, 0.0001)
    assert crank_offset.y == pytest.approx(sqrt2by2, 0.0001)
    crank_position = wheel.crank_position(radius=1, tilt_degrees=0)
    assert crank_position.x == pytest.approx(2+sqrt2by2, 0.0001)
    assert crank_position.y == pytest.approx(3+sqrt2by2, 0.0001)


def crank_position(self, radius, tilt_degrees):
    return self.crank_offset(radius, tilt_degrees) + self.constraint

Runs. Now we really ought to be able to use this, but I suspect that it is going to break the movie. We’ll see. Use it:

    def constrained(self, number):
        crank = self._constraint.crank_offset(self._radius, self._tilt_radians)
        x_proj = math.sqrt(self._length * self._length - crank.y * crank.y)
        constrained_x = crank.x + x_proj
        un_rotated = Vector((constrained_x, 0, 0))
        rotated = un_rotated.rotate_xy(self._tilt_radians)
        center = self._constraint.constrained(0) # constrained?
        return rotated + center

    def init_draw(self, canvas):
        target = self.constrained(0)
        crank = self._constraint.crank_position(self._radius, self._tilt_radians)
        self.line = canvas.create_line(
            crank.x, crank.y,
            target.x, target.y,
            width=3, fill="red")

    def draw(self, canvas):
        target = self.constrained(0)
        crank = self._constraint.crank_position(self._radius, self._tilt_radians)
        canvas.coords(self.line,
                      crank.x, crank.y,
                      target.x, target.y)

Tests all pass. And, as I expected, the ConRod is back to going around the circle opposite to the radial marker of the wheel.

Something makes me suspect that the wheel could be wrong. In any case, however it’s doing it, it should be using its new methods.

Well, look at this:

class Wheel:
    def draw(self, canvas):
        radius = 1
        center = self.constraint
        end = center + VtVector((radius, 0, 0)).rotate_xy(-self._angle_radians)
        canvas.coords(self.line, center.x, center.y, end.x, end.y)

Why is that - in there? Anyway we have a perfectly good method to use for this.

    def draw(self, canvas):
        radius = 1
        center = self.constraint
        end = self.crank_position(radius, 0)
        canvas.coords(self.line, center.x, center.y, end.x, end.y)

Now the picture is correct:

conrod

Tests pass. Commit.

Summary

We have removed a mutable member from ConRod, which leaves it simpler in the sense of not changing during operation, and improved its relationship with its constraint.

The costs of this improvement include:

  1. We have two new required methods in the relationship between a ConRod and its constraint, crank_offset and crank_position. Possibly we could improve that, but it seems to me that the ConRod needs both those related values.
  2. We are calling those methods more frequently, because we cached the value in a mutable member in ConRod. This is surely a bit less efficient.

Today, I like it better this way. We’ll see how it feels as we go along. As Hill would tell us, we are in the business of changing code, so if it needs changing, we’ll change it.

I think the tests are a bit weak, in that they aren’t really checking many variations of angle and tilt and such. I’m confident in the code, but when we change it, we rely on the tests to tell us things are still OK, and there are some gaping holes in the net. We’ll see what happens.

Overall, I’m pleased. It will be easy to do our next feature. I’d do it now but I’d rather have an iced chai and read my book.

See you next time!