Today let’s review the code and improve it a bit. I think PyCharm can help with that. Same refactoring, four times!

The operational part of the Rail class looks like this:

    def bounce(self, ball):
        if self.colliding(ball):
            vel = ball.velocity.rotate(self.angle)
            vel.y = -vel.y
            ball.velocity = vel.rotate(-self.angle)

    def colliding(self, ball):
        return not self.too_far(ball) and self.ball_is_approaching(ball)

    def too_far(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance > ball.radius

    def ball_is_approaching(self, ball):
        velocity = ball.velocity.rotate(self.angle)
        return velocity.y > 0

I see one little issue that could be improved. We have the method too_far but we only use it in the object as not too_far. It seems to me that it would be better to have close_enough. I think PyCharm wants to help, and I want to practice using its refactoring assistance.

Let’s extract a method.

Select not self.too_far(ball) and Option+Command+M to extract method. Call it close_enough.

    def colliding(self, ball):
        return self.close_enough(ball) and self.ball_is_approaching(ball)

    def close_enough(self, ball):
        return not self.too_far(ball)

I think there are some tests that use too_far. We’ll probably want to look into that. So far, auto_run says we’re green.

Now I want to inline the self.too_far(ball) in close_enough, but PyCharm can’t see how to do it. OK, we’ll start here:

    def too_far(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance > ball.radius

Extract variable on the return statement, Option+Command+V:

    def too_far(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        is_too_far = y_distance > ball.radius
        return is_too_far

Copy those three lines into close_enough and use the value:

    def close_enough(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        is_too_far = y_distance > ball.radius
        return not is_too_far

Inline is_too_far, Option+Command+N:

        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return not y_distance > ball.radius

Look for a yellow light bulb offering to reverse the conditional. Hard to find but if you set the cursor on the < PyCharm offers to “Negate >”, giving:

    def close_enough(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance <= ball.radius

And that’s nearly good. Except that I see some duplication:

    def close_enough(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance <= ball.radius

    def too_far(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        is_too_far = y_distance > ball.radius
        return is_too_far

Clearly we can now use close_enough to implement too_far. Can we get PyCharm to do it? As much as I’m enjoying learning its tricks by searching out refactoring steps it can help with, I’m getting bored. We all know that we could have done this by hand pretty easily.

Yeah, I don’t see it. Just do it:

    def too_far(self, ball):
        return not self.close_enough(ball)

We are green. That was fun, but not as smooth as I’d have hoped. I think IDEA with Kotlin could have done more refactorings automatically: it’s easier, in a sense, with a language with stronger typing.

I’m going to revert and do this again another way.

Another Way

We know we want close_enough. Looking at this method:

    def too_far(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance > ball.radius

We can see that if it returned not y_distance > ball.radius, it would be able to be named close_enough. We could copy it and do that. Let’s try that path and see if it feels better. I think it might be.

    def close_enough(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return not y_distance > ball.radius

Find the hint and let PyCharm change the not:

    def close_enough(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance <= ball.radius

Now we can change too_far to use close_enough:

    def too_far(self, ball):
        return not self.close_enough(ball)

And we have to remember to change colliding:

    def colliding(self, ball):
        return self.close_enough(ball) and self.ball_is_approaching(ball)

Now here, it was easy to remember. Although it would also have been easy to forget. Plus, the changes were all done manually, except for reversing the >. Risky, but it went smoothly enough. Still, I’d prefer to use only automated refactoring steps if I could. It’s more reliable and generally faster.

Let’s back it out again and try another path. I’m trying to learn the tools. The code is well in hand.

Yet Another Way

Start here:

    def too_far(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance > ball.radius

Extract the two operational lines to a new method, Option+Command+M:

    def too_far(self, ball):
        y_distance = self.distance_to_rail(ball)
        return y_distance > ball.radius

    def distance_to_rail(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance

Inline that variable in too_far, Option+Command+N:

    def too_far(self, ball):
        return self.distance_to_rail(ball) > ball.radius

Oh this might work, and if it does it’ll be quite nice. Click on the > and Negate.

    def too_far(self, ball):
        return not self.distance_to_rail(ball) <= ball.radius

And now extract the comparison but not the not … to close_enough! Option+Command+M:

    def too_far(self, ball):
        return not self.close_enough(ball)

    def close_enough(self, ball):
        return self.distance_to_rail(ball) <= ball.radius

Nice! Automated all the way. I’m glad I tried again and again.

Pat on the back for me.

Some authors would erase the first two tries and show you the third really nice one saying “See, that’s how you do that”. Not me: I’m saying that this is how you do that: you try one thing, another thing, a third thing, and you improve. And, of course, you can give up any time you want.

I think we should notice what made this sequence easy, looking at this version:

    def too_far(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance > ball.radius

Here, the two operational lines were a concept of their own, the distance to the rail, and extracting that as a function seems to have been key to the refactoring. We could inline it back in, but I was just thinking that and realized that it expresses an idea that was in my head (get the distance to the rail), and so it belongs in the code: “Expresses all the programmer’s ideas”.

There was still that clever step of negating the conditional, in too_far, seeing that if negated it would be close_enough. I don’t know where that idea came from … but I’m pretty sure that if I hadn’t gotten to here:

    def too_far(self, ball):
        return self.distance_to_rail(ball) > ball.radius

I’d never have gotten here:

    def too_far(self, ball):
        return not self.distance_to_rail(ball) <= ball.radius

If there’s a pattern that I’m following here, it’s not a very explicit one. It’s pretty much a “try things, follow your nose” sort of pattern. I try to change the code to be more like what I need. I let myself make small steps that are more or less in the right direction. And, of course, it helps to be willing to roll back and try again.

If I were under pressure to “get done”, I’d not have done this. That pressure could be applied externally, by management, or by lunch-time looming, but I find that the less pressure I feel, the more smoothly things seem to go.

However: I still have to remember to use the new method here:

    def colliding(self, ball):
        return not self.too_far(ball) and self.ball_is_approaching(ball)

We want:

    def colliding(self, ball):
        return self.close_enough(ball) and self.ball_is_approaching(ball)

The first attempt at this refactoring began by automatically creating the close_enough method, but I couldn’t find an automated way to refactor it into the new form.

I suppose that a better man than I would try a fourth time. Oh, heck, why not? Roll back one more time.

Fourth Time

Begin by extracting close_enough:

    def colliding(self, ball):
        return self.close_enough(ball) and self.ball_is_approaching(ball)

    def close_enough(self, ball):
        return not self.too_far(ball)

Now, I should mention that on a given day I might just leave that. I’m not afraid of a method call, and might be willing to pay the price for the improved clarity. But we want to get to the point where we have all the logic in close_enough and too_far calling it. So what we really want to do is inline too_far into close_enough. Last time I couldn’t find a way.

This time we’ll create the distance method:

    def too_far(self, ball):
        y_distance = self.distance_to_rail(ball)
        return y_distance > ball.radius

    def distance_to_rail(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance

Inline those two explaining variables? Or not? Inline the first one:

    def too_far(self, ball):
        return self.distance_to_rail(ball) > ball.radius

Negate the conditional. The light bulb will do it.

    def too_far(self, ball):
        return not self.distance_to_rail(ball) <= ball.radius

Now can we inline the code in close_enough:

    def close_enough(self, ball):
        return not self.too_far(ball)

PyCharm can’t see how to inline that call. It’s so obvious. I can copy and paste it:

    def close_enough(self, ball):
        return not not self.distance_to_rail(ball) <= ball.radius

Maybe there was a way to hold my mouth that would have automated that, but it’s pretty safe.

I think we might get some help on the nots. Curiously, we do not. Another manual step:

    def close_enough(self, ball):
        return self.distance_to_rail(ball) <= ball.radius

We could leave things as they are now … but I’d rather fix too_far:

    def close_enough(self, ball):
        return self.distance_to_rail(ball) <= ball.radius

    def too_far(self, ball):
        return not self.distance_to_rail(ball) <= ball.radius

I wonder if PyCharm can extract that inequality now and detect both to remove:

Yes. I extract close_enough_2 and get this:

    def close_enough(self, ball):
        return self.close_enough_2(ball)

    def close_enough_2(self, ball):
        return self.distance_to_rail(ball) <= ball.radius

    def too_far(self, ball):
        return not self.close_enough_2(ball)

This certainly seems like three rights to make a left but let’s see if it can do the inline now.

If I put the cursor on close_enough_2 it is smart enough to put it back the way it was. That’s no help, I’m back to here:

    def close_enough(self, ball):
        return self.distance_to_rail(ball) <= ball.radius

    def too_far(self, ball):
        return not self.distance_to_rail(ball) <= ball.radius

OK, of course I could just do it but I want to bend PyCharm to my will here. I tried inlining too_far but that just plugs that expression in everywhere all over my tests. Not what I wanted.

I don’t see it. I’ll just change it. My tests will protect me anyway.

    def too_far(self, ball):
        return not self.close_enough(ball)

Well, here we are again. Four paths to the same result. I kind of prefer the paths that started by extracting close_enough immediately, but they seemed to involve more manual steps. I’ll leave an open question, whether there is a refactoring from too_far to close_enough that leaves too_far calling close_enough, and using all automated refactorings.

For now, we’ve got what I wanted. Let’s reorder the code a bit and review it:

class Rail:
    @classmethod
    def foot(cls, dist):
        return Rail(0, dist, 180)

    @classmethod
    def head(cls, dist):
        return Rail(0, dist, 0)

    @classmethod
    def right(cls, dist):
        return Rail(0, dist, 90)

    @classmethod
    def left(cls, dist):
        return Rail(0, dist, -90)

    def __init__(self, x, y, angle):
        self.position = Vector2(x,y)
        self.angle = angle
        names = {0: "foot", 180: "head", 90: "left", -90: "right"}
        self.name = names[angle]

    def bounce(self, ball):
        if self.colliding(ball):
            vel = ball.velocity.rotate(self.angle)
            vel.y = -vel.y
            ball.velocity = vel.rotate(-self.angle)

    def colliding(self, ball):
        return self.close_enough(ball) and self.ball_is_approaching(ball)

    def close_enough(self, ball):
        return self.distance_to_rail(ball) <= ball.radius

    def distance_to_rail(self, ball):
        pos = ball.position.rotate(self.angle)
        y_distance = self.position.y - pos.y
        return y_distance

    def ball_is_approaching(self, ball):
        velocity = ball.velocity.rotate(self.angle)
        return velocity.y > 0

    def too_far(self, ball):
        return not self.close_enough(ball)

OK, I like that. Maybe the name bounce should be bounce_off_rail_if_so_inclined or something, but I think we’ll leave it at bounce. We could extract a method from bounce if we wish:

    def bounce(self, ball):
        if self.colliding(ball):
            self.invert_y_velocity(ball)

    def invert_y_velocity(self, ball):
        vel = ball.velocity.rotate(self.angle)
        vel.y = -vel.y
        ball.velocity = vel.rotate(-self.angle)

That’s probably more communicative. Is the reference to “y” confusing? Should we do this?

    def bounce(self, ball):
        if self.colliding(ball):
            self.reflect_off_rail(ball)

    def reflect_off_rail(self, ball):
        vel = ball.velocity.rotate(self.angle)
        vel.y = -vel.y
        ball.velocity = vel.rotate(-self.angle)

Small gains. Enough for now.

Summary

We have tried four successful approaches and written many lines improving a “not”. Worth doing? “Not” in terms of this code, perhaps, but in terms of working out how to do it, practicing our skills, learning our new tool PyCharm, I think so. I think we’re pretty close to publication quality code here. I hope so, because I’m going to publish it. Of course, you’ve seen some of the stuff I’ll publish. I’m about warts and all here, not code that only some god could create.

The original mission was just to create a Rail class that could serve as a rail on all four sides of a billiard table. I think we’ve accomplished that. The “trick” is to rotate the ball’s coordinates so that the rail always sees itself as if it were the head rail, and measures ball position and motion accordingly. That works sweetly.

We could make our Rail ready to serve for a pool table by adding a check for the x coordinate being within the end points of a shorter rail. That change is “left to the reader”.

I think we’re done here. See you next time!