The final of four articles covering a single refactoring session. It has gone ever so nicely.
One more code review … unless we see something we can improve. We’ll kind of step through the code, asking questions and getting answers. When the answers are hard to get, that’s a hint that the code could use more improvement.
def fire_available_missile(self, chance, fleets, saucer, ship_or_none): if ship_or_none and self.should_target(chance, saucer): self.create_optimal_missile(fleets, saucer, ship_or_none) else: self.create_random_missile(fleets, saucer)
What do we mean by
@staticmethod def should_target(chance, saucer): return saucer.always_target or chance < u.SAUCER_TARGETING_FRACTION
Either we always target or the odds were not in our favor. OK. Now what is a random missile?
def create_random_missile(self, fleets, saucer): target = self.random_position() self.create_unoptimized_missile(saucer.position, target, fleets)
Ah. Just an unoptimized missile to a random position. Makes sense. What about an optimal missile?
def create_optimal_missile(self, fleets, saucer, ship): saucer_position = saucer.position closest_shot_position = self.closest_aiming_point(saucer_position, ship.position, u.SCREEN_SIZE) delta_position = closest_shot_position - saucer_position aim_time, adjustment_ratio = self.optimal_shot(delta_position, ship) target_position = closest_shot_position + ship.velocity * aim_time self.create_adjusted_missile(adjustment_ratio, target_position, saucer_position, fleets)
OK, this isn’t perfectly clear. Probably needs improvement. Must think about that, but clearly it figures out a position and an adjustment and creates an adjusted missile. How does it do that?
def optimal_shot(self, delta_position, ship): aim_time = self.time_to_target(delta_position, ship.velocity) adjustment_ratio = self.compute_adjustment_ratio(aim_time) return aim_time, adjustment_ratio def compute_adjustment_ratio(self, aim_time): return self.compute_ratio(aim_time) if aim_time else 1 def compute_ratio(self, aim_time): distance_to_target = aim_time * u.MISSILE_SPEED adjusted_distance = distance_to_target - 2 * self._radius return adjusted_distance / distance_to_target
Well, it gets time to target and some adjustment ratio. Would renaming compute_ratio help here? Maybe
compensate_for_offset? We’ll do that now.
- As we review code, we’ll often get an idea for a better name. I think it’s good to take that opportunity, particularly when the change is essentially local.
def optimal_shot(self, delta_position, ship): aim_time = self.time_to_target(delta_position, ship.velocity) adjustment_ratio = self.velocity_adjustment(aim_time) return aim_time, adjustment_ratio def velocity_adjustment(self, aim_time): return self.compensate_for_offset(aim_time) if aim_time else 1 def compensate_for_offset(self, aim_time): distance_to_target = aim_time * u.MISSILE_SPEED adjusted_distance = distance_to_target - 2 * self._radius return adjusted_distance / distance_to_target
Moving on, there’s just
create_adjusted_missile left to review:
def create_adjusted_missile(self, adjustment_ratio, target_position, saucer_position, fleets): vector_to_target = target_position - saucer_position direction_to_target = vector_to_target.normalize() missile_velocity = u.MISSILE_SPEED * direction_to_target adjusted_velocity = missile_velocity * adjustment_ratio offset = 2 * self._radius * direction_to_target missile = Missile.from_saucer(saucer_position + offset, adjusted_velocity) fleets.append(missile)
That could use a rename of
def create_adjusted_missile(self, velocity_adjustment, target_position, saucer_position, fleets): vector_to_target = target_position - saucer_position direction_to_target = vector_to_target.normalize() missile_velocity = u.MISSILE_SPEED * direction_to_target adjusted_velocity = missile_velocity * velocity_adjustment offset = 2 * self._radius * direction_to_target missile = Missile.from_saucer(saucer_position + offset, adjusted_velocity) fleets.append(missile)
There’s certainly a lot going on here, but it seems pretty well organized. Mostly we do not need to drill down to see what’s going on.
I still suspect that an additional object or two might help us here, but for the morning, I think we have accomplished a lot. Let’s sum up.
This single-morning session, of about three hours, has blossomed into four articles, mostly because I’m showing all the code changes. The changes themselves are often only a few lines, or an extract.
What has just happened here is a very long series of simple changes, aimed at improving the design of the Gunner. The changes have been almost all PyCharm refactorings, which are (nearly) guaranteed not to break anything, plus a few very simple code manipulations done by Yours Truly in small steps and with care.
We did begin with some experimental code (a spike) and when it worked, I was confident enough to commit it. Many folks would argue that we should never commit a spike. I agree with them, so right before I commit a spike I rename it to a “tentative step forward”. Seriously, though, it requires judgment, brains, and maturity to decide what to do with a spike, and throwing it away is always a good option. What should you do? You should use your own judgment, not mine.
Then I did a real refactoring all by my very own self, changing the way that the missile velocity is computed, converting from using an angle and rotating it, to computing a direction vector and scaling it. Simple and accurate well-known mathematical transformation.
There would be no shame in not knowing that much vector math, although if one is going to work in outer space like this, it’s a valuable skill to have. But it was precisely a refactoring, as it didn’t change the results. The tests even ran before and after, and in between they found some small errors in my code, just like tests are supposed to do.
I think that at that point I should have committed, because it would have been a quite decent save point, but as things turned out, I didn’t need a save point.
One of my biggest flaws in this work in general, and certainly in this session, is that I do not commit the code as often as would be ideal, which sometimes results in a need to go back further than I would like. Today’s limited commits are in part due to the fact that I always commit to GitHub. If I didn’t do that, it might be easier to revert locally. But mostly, I just don’t think to commit as often as I could. Many years of bad habits.
This article series is quite long, but it reflects just a single work session. Throughout the whole period, tests stayed green except when they were actually finding new typos and mistakes. Commits were frequent and could have been more frequent if i were a better person.
I hope that breaking up the article helped a bit. I’m not sure what could make this sequence of events as simple for you, the reader, as it was for me, the programmer / writer. It was amazingly smooth and without stress.
Why has it gone notably well?
We might ask why it sent so well. I think there are two reasons:
First, this is day three or four of looking at this code and making small improvements, culminating this morning when I saw how to patch in a transient member variable that carried important information forward from a method that knew the facts to a method that needed the facts.
That change, done first thing this morning, introduced temporal coupling into the object, but it was quite clear that it worked. A bit nasty but correct.
Today, my overall scheme was to keep pushing the separate methods back together with inlining, until the code that knew the fact and the code that needed the fact were together, and then to split things back apart along new lines that kept the fact local and therefore no longer troublesome.
What is troubling about this seemingly directionless approach, at least to some, is that there is no particular reason to believe there will be a good outcome, and along the way, there is the risk that we’ll become confused and be unable to proceed, or that we’ll break something terribly and not be able to fix it.
I avoided the fear by virtue of a number of elements:
- I wasn’t required to succeed. I maintained an experimental frame of mind and a willingness to revert and think about what I had learned;
- I was going in very small steps, most of which were automated, and all of which felt like decent moves on their own … except for the actual inlining ones;
- The inlining ones didn’t bother me because they were all machine-refactorings except for one simple one, so I was sure they worked;
- I am confident that I can improve long procedures with suitable extractions and name changes, because I’ve done a lot of that.
- My tests have been solid and supportive, and remained so.
But it’s certainly fair to say that I didn’t step in small steps each leading to better code. On the contrary: while many of the steps did make the code better, some, the inlining of whole methods, certainly did not. Why is it OK to make the code worse?
It’s OK to make the code worse because we can always reverse the changes if we wish, and because by putting the lines back together, they allow us to reorder the sequence of events, and discover new meaningful bits, so that subsequent extractions lead to an even better situation than the one we stepped away from.
Sometimes you go backward to go forward. The thing is not to be afraid to try.
In short, I think that my comfort today came down to two elements:
- The more experience one gets with small steps, the more confident one can be in taking small steps, even backward;
- Good tests are critically helpful.
Without those tests … there’s no way I would have been comfortable doing all this. As for the experience … the trick is to do lots of sequences of two or three or four small steps off the path and back, to build up confidence about being off the path.
I do apologize for the length of this series. In a sense, these past four articles are all of a piece, because they reflect the work of a single programming session. And, by my lights, a very rewarding one.
If you’re still awake, I hope you’ll come back next time. If you’re not still awake, I hope you’re not holding down a key with your forehead. GGGGGGGGGGGGGGGG …
See you next time!