Jira is the name of the area on my keyboard tray where the yellow sticky notes live. It’s getting messy. Let’s see if there’s anything on there we can scratch off, or if all else fails, actually do.
Some of the stickies refer to larger efforts, not specifically asteroids, things I might want to study, write about, or just do:
- Make PyGame template w/ Game class
- Events ala Rickard Lindberg
- Nullables per James Shore
Here are the Asteroids-specific items:
- Testing flag in ScoreKeeper / Fonts object
- Saucer visible explosion
addAsteroid vs fleets.asteroid.
- move -> update (tick?)
- extra parms in tick move saucer
- Simplify Saucer
- hyperspace refractory period
- Flyer -> (Drawable, Invisible)
- Star field
- Debug keys
Saucer uses fleets.ships just need ship/None
- Missile vs missile
check range in objects
I’ve put strikeout marks on the ones that are done or no longer applicable because of other changes. I think I decided not to worry about double hits. If they happen, they happen. Or maybe I fixed it. Either way, I’m dropping the item.
Let’s look at that first one. It might be easy.
class ScoreKeeper(Flyer): def __init__(self, testing=True): self.score = 0 if not testing: self.score_font = pygame.font.SysFont("arial", 48)
There are twenty usages of ScoreKeeper, mostly in the tests, all creating it with no parameter. The Quarter object creates it with an explicit False, so that it will create the Font.
We’ll have to test this in Game, or, let me be more honest, testing this in Game seems easier than writing a test for it, and will be quite secure.
The reason we have this check is that during testing, pygame is not initialized and so you can’t go about grabbing fonts. Without the flag, we get 14 tests failing, saying this:
> font = Font(fontpath, size) E pygame.error: font not initialized
However, I’ve now learned that you can use
class ScoreKeeper(Flyer): def __init__(self, testing=True): self.score = 0 if pygame.get_init(): self.score_font = pygame.font.SysFont("arial", 48)
And with this in place, the tests run just fine. So we can remove the parameter and the single user of it. This:
class Quarter(Flyer): def tick(self, delta_time, fleet, fleets): fleets.clear() fleets.add_flyer(SaucerMaker()) fleets.add_flyer(ScoreKeeper(False)) fleets.add_flyer(ShipMaker()) fleets.add_flyer(Thumper()) fleets.add_flyer(WaveMaker())
class Quarter(Flyer): def tick(self, delta_time, fleet, fleets): fleets.clear() fleets.add_flyer(SaucerMaker()) fleets.add_flyer(ScoreKeeper()) fleets.add_flyer(ShipMaker()) fleets.add_flyer(Thumper()) fleets.add_flyer(WaveMaker())
class ScoreKeeper(Flyer): def __init__(self): self.score = 0 if pygame.get_init(): self.score_font = pygame.font.SysFont("arial", 48)
Tests good. Commit: Remove testing flag from ScoreKeeper.
I toss that sticky out.
I notice the “Saucer visible explosion” one. When the saucer crashes or gets shot, there’s a sound but it just vanishes. Let’s give it an explosion, but not with the little spaceman. We’ll do that in stages, first with the standard explosion.
Review Ship to see how it does it.
class Ship(Flyer): def interact_with_missile(self, missile, fleets): self.tally_ship_missiles(missile) self.explode_if_hit(fleets, missile) def explode_if_hit(self, fleets, attacker): if attacker.are_we_colliding(self.position, self.radius): self.explode(fleets) def explode(self, fleets): player.play("bang_large", self._location) fleets.remove_flyer(self) fleets.add_flyer(Explosion(self.position))
We get the idea. Now let’s look at the saucer’s demise.
class Saucer(Flyer): def interact_with_missile(self, missile, fleets): if missile.is_saucer_missile: self._missile_tally += 1 if missile.are_we_colliding(self.position, self.radius): fleets.add_flyer(Score(self.score_for_hitting(missile))) self.explode(fleets) def explode(self, fleets): player.play("bang_large", self._location) player.play("bang_small", self._location) fleets.remove_flyer(self)
It seems pretty clear that adding the explosion right after that last line is just the thing. I just want to see it happen, so I’ll add the vanilla explosion first.
def explode(self, fleets): player.play("bang_large", self._location) player.play("bang_small", self._location) fleets.remove_flyer(self) fleets.add_flyer(Explosion(self.position))
Works as advertised. Now let’s see how Explosion works. We know it adds Fragments which have a few shapes.
class Explosion: def tick(self, delta_time, fleet, fleets): fleets.remove_flyer(self) self.explosion_at(self.position, fleets) def explosion_at(self, _position, fleets): simple = Fragment.simple_fragment vee = Fragment.v_fragment guy = Fragment.astronaut_fragment fragment_factory_methods = [vee, guy, simple, simple, simple, simple, simple] random.shuffle(fragment_factory_methods) how_many = len(fragment_factory_methods) for i in range(how_many): factory_method = fragment_factory_methods[i] base_direction = 360 * i / how_many self.make_fragment(factory_method, base_direction, fleets) def make_fragment(self, factory_method, base_direction, fleets): twiddle = random.randrange(-20, 20) fragment = factory_method(position=self.position, angle=base_direction+twiddle) fleets.add_flyer(fragment)
I think this would go best with two constructors for Explosion, one providing the fragment method list for ship, one for saucer. We’ll do that with class methods.
class Explosion(Flyer): @classmethod def from_ship(cls,position): simple = Fragment.simple_fragment vee = Fragment.v_fragment guy = Fragment.astronaut_fragment return cls(position, [vee, guy, simple, simple, simple, simple, simple]) @classmethod def from_saucer(cls,position): simple = Fragment.simple_fragment vee = Fragment.v_fragment return cls(position, [vee, vee, simple, vee, simple, vee, simple]) def __init__(self, position, fragment_factory_methods): self.position = position self.fragment_factory_methods = fragment_factory_methods def explosion_at(self, _position, fleets): random.shuffle(self.fragment_factory_methods) how_many = len(self.fragment_factory_methods) for i in range(how_many): factory_method = self.fragment_factory_methods[i] base_direction = 360 * i / how_many self.make_fragment(factory_method, base_direction, fleets)
And the Ship and the Explosion test call
from_ship and Saucer calls
from_saucer and it works as intended. The saucer now explodes with four vee shapes and three straight ones. We could fancy the explosions up one way or another if we wanted.
Commit: saucer now has fancy explosion. Another sticky note bites the dust.
Could we have done that a different way? Yes. For example, we could have added the fragment list parameter to Explosion and defaulted it for ship, and provided it for Saucer. Then we would not have had to change Ship or the test.
But a Ship and Saucer have no real business knowing what their explosion should look like. So they should tell Explosion what to do, not ask.
We could have provided a flag for them to set. Meh. That’s just tacky if you ask me. And you didn’t, but I did.
We could have done this with two or three commits rather than just the one. You’ve got me there: I definitely did more work between commits than was absolutely necessary. I have a habit of doing that, and it’s not a good habit.
Now I hope you’ll agree that the few lines I changed aren’t a really big risky deal, but I definitely could have done it this way:
- Add the parameter. Ignore it. Commit.
- Default the parameter. Ignore it. Commit.
- Use the parameter in
- Create the class method. Test it (there is a test). Commit.
- Use the class method in Ship. Commit.
- Create the new Saucer class method. Don’t use it. Commit.
- Use it. Commit.
Now that might even be too many commits for Hill, but a sequence like 1,2; 3; 4,5; 6,7; might have been safer. I kind of wish I had done it that way, just to impress you or make you wonder how paranoid I could really be.
Seriously, given the few moments it takes to run my tests and commit a file or two, I would not be slowed down by a sequence like that.
It is my practice to push every commit, to the point where I just punch out Command+k, Command+v, Command+Option+k and there it goes. That makes it a hassle between me and GitHub if I want to rollback or revert or whatever it’s called. Perhaps if I were to spend a little time working out how best to undo a few commits and start from there, I’d be more inclined to tinier commits.
I could be better. I could know more things, be more expert with more of my tools. But at any moment in time, we know what we know, we choose where to invest in learning, and sometimes we pry something with a screwdriver because we can’t find the pry bar.
We’re just imperfect humans in a big big world of weird tools. And that’s OK by me.
We’re at a respectable 250 lines of input here. Is there anything else we might like to do?
I see this one note that says that
tick has an extra parameter.
class Flyer: @abstractmethod def tick(self, delta_time, fleet, fleets): pass
I think that no one should be using the fleet parameter and that it should be able to be removed. Let’s search to find out. There are 15 implementations of
Asteroid and Explosion do not touch that parameter. Fragment does:
class Fragment(Flyer): def tick(self, delta_time, fragments, _fleets): self.timer.tick(delta_time, self.timeout, fragments) def timeout(self, fragments): fragments.remove(self)
That can be done differently, and should be:
def tick(self, delta_time, _fragments, fleets): self.timer.tick(delta_time, self.timeout, fleets) def timeout(self, fleets): fleets.remove_flyer(self)
I’ll test and commit that: Fragment does not use fragments parameter in tick.
Missile has a similar issue. It gets similar treatment and a test needs help. Check GitHub if you want to see something just like what’s above: Commit: “Missile does not use fleet parameter in tick.”
- I was expecting that no one would be using that parameter. No one should be using it. I’m not even entirely sure why it works if they do use it. So while I didn’t expect these changes, I do think they should be made. Doing them one by one keeps it safe.
Quarter is OK. Saucer is OK. SaucerMaker is OK. Score is OK. ScoreKeeper is OK. Ship is OK. ShipMaker is OK. The BeginChecker and EndChecker test objects are OK and will need to be changed, perhaps manually, when we do the refactoring.
Thumper is OK. Couldn’t find Bambi. WaveMaker is OK.
Now let’s see if PyCharm can do this refactoring without destroying the universe. I have my doubts.
We Change Signature in the abstract method in Flyer:
@abstractmethod def tick(self, delta_time, fleets): pass
Tests are all green, so that’s a good sign. Now to check all the implementations in the subclasses because I know it’s possible for PyCharm to get confused during this operation.
PyCharm has done a perfect job, even found the ones in the tests. I apologize for ever doubting you, PyCharm but things have gone awry in the past. I suspect that it has gone so well because all those implementations are in identified subclasses of Flyer, so PyCharm knows they’re all supposed to have the same signature. Well done.
- Lesson to Learn
- In Python, even given duck typing, if we’re thinking of some objects as members of some common idea, giving them a common superclass, and some abstract methods, can pay off by helping PyCharm do a better job, making the code a bit more clear, and quite likely making it more reliable, especially if we do some tests to check compliance.
Commit: remove fleet parameter from
tick method throughout.
That was a 20-file refactoring, done with one command in PyCharm. These are the days when a tool like PyCharm is a great thing to have. It would have been tedious at best to do that refactoring manually, and risky as well. Go PyCharm!
I think we’ve now had a very productive morning, it’s only 0925, and we’ll sum up and publish.
We removed a flag from a class’s init, simplifying use and avoiding a “What’s that False there for” question every time anyone read the Quarter object. It was easy because pygame has a safe
getinit call that you can make.
Then we made a nice explosion visual effect for the Saucer, which entailed creating two constructor methods so that the ship and saucer can have different-looking explosions. That went well, although I could have gone in smaller steps.
Then we removed a needless parameter from the very commonly used
tick method, increasing clarity and consistency. We first changed any implementor of
tick who was using that parameter to use the
fleets parameter instead. Then in one swell foop, PyCharm refactored all the implementations and calls to remove that parameter. Very fine.
Big Picture Thoughts
First, I would bet that those of you who push code for a living do not have a lot of time available for improving the code in small ways like we did this morning. You’re under pressure to ship features or whatever random pressure you’re under. But please note: these changes do improve the code, and take very little time. Since simpler code is easier to understand, improving it in areas that we revisit pays off. So I wouldn’t go looking for places to improve, on my real job, but when I was passing through something with another purpose, I would improve a few things while I’m there. I’ve found that that pays off in easier development.
Second, I find it fascinating that even after over 100 days of very part-time working on this program, publishing it for people to see and criticize, I continue to find things that can be improved. I try pretty hard to write good, clear code, with useful tests. And yet, every day, it seems that there is room for improvement.
I think that’s great. I get a kick out of finding a way to make things better. And doing so keeps my skills well honed, and gives me ideas for how to do everything just a bit better.
That’s great, because I have six decades of bad habits to improve. I hope it will go the same for you!
See you next time!
- As I was reviewing the displayed version of this article before pushing it, I noticed that Explosion knows the method names from Fragment,
simple. I wonder if it might be better to use some shared string names or a kind of enum or something like that.
See what I mean about there always being some candidate for improvement?