Python 81
I need a distraction. Let’s look for one.
Let’s check the properties that we added to the flyers. We still have, variously, accessors and setters for position and velocity. If the usage is for tests, we might find that we can rely on the tests for MovableLocation, and access from the objects themselves may offer opportunities for refactoring. (It turns out that I don’t look into this further. Probably should, sometime.)
Asteroid class only offers position. The first reference PyCharm finds is this one:
class Asteroid:
def draw(self, screen):
top_left_corner = self.position - self.offset
screen.blit(self.surface, top_left_corner)
Given the limitations of pygame’s graphics, I don’t see a great deal to do about that. If pygame supported translate, like a graphics context oriented graphics system, we could ask the ML to translate to its current position. As things stand, not much to do here.
As a separate issue, one might argue that the drawing of the objects should be more full separate from the logic of flying in space and colliding and the like. If and when we do that, opportunities may arise.
The next accesses are involved in creating new split asteroids:
def split_or_die(self, asteroids):
if self not in asteroids:
return # already dead
asteroids.remove(self)
if self.size > 0:
a1 = Asteroid(self.size - 1, self.position)
asteroids.append(a1)
a2 = Asteroid(self.size - 1, self.position)
asteroids.append(a2)
We could imagine a creation method for Asteroid that took a MovableLocation as a parameter. I’m not feeling it. It might be as simple as
a1 = Asteroid.from_location(self.location, self.size - 1)
But that method would still know how to get the position. I think that since we can make MovableLocation.position a protected property if we need to, we can let this kind of access go.
class Asteroid:
def within_range(self, point, other_radius):
dist = point.distance_to(self.position)
return dist < self.radius + other_radius
This one is interesting. Is this method duplicated in other classes? No. In fact PyCharm says it is not used. There are, in fact, copies of this in two of the four the flyers. Let’s make sure we have no changes buffered and then try removing them.
Ha! Detritus left over from before Collider, I guess. Commit: remove unused within_range methods.
What else is unused? Does PyCharm have a way of looking for such things? It does not, yet it will tell me when things are not accessed. I think the concern is that there are some very tricksy ways of accessing methods that PyCharm can’t detect.
OK, I’ve examined all the references to position in Asteroid and think I’ll allow them. There are some other references outside Asteroid that may be interesting. There is a live function within_range that looks a bit like it could use improvement:
class Collider:
@staticmethod
def within_range(target, attacker):
in_range = target.radius + attacker.radius
dist = target.position.distance_to(attacker.position)
return dist <= in_range
We could defer the distance_to to ML, which would move us toward making the position property at least private to the flyers.
distance_to is a function on Vector2. Anyway we could say this:
@staticmethod
def within_range(target, attacker):
in_range = target.radius + attacker.radius
dist = target.location.distance_to(attacker.location)
return dist <= in_range
Either way it’s kind of invasive. It is tempting to put within_range back into all the flyers, but that creates duplication. Is it better to refer to the location rather than the position?
Design Digression
When we create objects, we always have to decide what properties and methods they will have. It seems to me that if I were a universe, the things in me would have a position and a velocity (which could perhaps not be perfectly known, thanks Dr Heisenberg). I would not think, “Oh, these things have a MovableLocation, which amounts to a position plus a velocity”.
The MovableLocation object, from this viewpoint, is not a natural object to refer to. Instead, it is a programmer’s hack to avoid duplication of code, specifically the move code that was at first all we had in ML.
Seen from this angle, I think that read accessors for position, and perhaps for velocity, are reasonable: we can imagine that some kind of space radar sees your position and can perhaps measure your velocity.
From this viewpoint, I think that it may be “better” if our objects consider each other in terms of position and velocity, provided by accessor methods, rather than knowing that each of them happens to have a location object inside. Position and velocity are properties of the things, ML is an implementation.
I’m glad we had this little chat. Now I’m going to go hunting for code accessing location and see about fixing that.
In asteroid, I decide to do something that I’ve not done before in Python. I’m going to mark some members “private”, which one does in Python by giving them a name starting with underbar. Private is not enforced by the language, but I’m sure PyCharm will hint about it.
class Asteroid(Flyer):
def __init__(self, size=2, position=None):
super().__init__()
self.size = size
if self.size not in [0, 1, 2]:
self.size = 2
self.radius = [16, 32, 64][self.size]
position = position if position is not None else Vector2(0, random.randrange(0, u.SCREEN_SIZE))
angle_of_travel = random.randint(0, 360)
velocity = u.ASTEROID_SPEED.rotate(angle_of_travel)
self._location = MovableLocation(position, velocity)
self._offset = Vector2(self.radius, self.radius)
self._surface = SurfaceMaker.asteroid_surface(self.radius * 2)
I think I might like that. Asteroid only implements position as a property. I wonder if the others can be driven to that point.
Commit: mark member variables private.
Let’s do Missile.
class Missile:
def __init__(self, position, velocity, missile_score_list, saucer_score_list):
self.score_list = missile_score_list
self.radius = 2
self._timer = Timer(u.MISSILE_LIFETIME, self.timeout)
self._saucer_score_list = saucer_score_list
self._location = MovableLocation(position, velocity)
I’m starting to like this convention.
Missile does have a velocity property. Why? It is used entirely in tests. Let’s rename it:
@property
def velocity_testing_only(self):
return self._location.velocity
And six tests. I think I like that as well. We’ll see.
In rare cases, the velocity of a flyer might not be private. I think we have no such cases now, but if the Saucer wants to “lead” the Ship when firing at it, we’ll need to take ship velocity into account. We’ll deal with that if we do the feature.
I think we can move on to Saucer. I decided to do this:
class Saucer:
def __init__(self, _position=None, size=2):
self.radius = 20
self.size = size
Saucer.direction = -Saucer.direction
x = 0 if Saucer.direction > 0 else u.SCREEN_SIZE
position = Vector2(x, random.randrange(0, u.SCREEN_SIZE))
velocity = Saucer.direction * u.SAUCER_VELOCITY
self._location = MovableLocation(position, velocity)
self._directions = (velocity.rotate(45), velocity, velocity, velocity.rotate(-45))
self.create_surface_class_members()
self.set_firing_timer()
self.set_zig_timer()
@property
def position(self):
return self._location.position
@property
def _velocity(self):
return self._location.velocity
@property
def velocity_testing_only(self):
return self._velocity
I did two properties for velocity. One is private, because it’s used inside the class, and the other, for testing. I did it that way to avoid a lot of warnings in the tests. It may have been better to just access the private version and suppress warnings, but we’ll try this.
Oops, I forgot to commit the Missile. Commit: Missile and Saucer set up private and testing-only members.
Now Ship.
I go hog-wild with the privates:
class Ship(Flyer):
def __init__(self, position):
super().__init__()
self.radius = 25
self._location = MovableLocation(position, Vector2(0, 0))
self._can_fire = True
self._angle = 0
self._acceleration = u.SHIP_ACCELERATION
self._accelerating = False
ship_scale = 4
ship_size = Vector2(14, 8)*ship_scale
self._ship_surface, self._ship_accelerating_surface = SurfaceMaker.ship_surfaces(ship_size)
There is a remote access to _angle, creating the available ships but I will allow that until PyCharm objects.
Now for position and velocity.
There are two internal references to velocity’s setter. We can fix them both. One:
def accelerate_by(self, accel):
self.velocity = self.velocity + accel
That should be:
def accelerate_by(self, accel):
self._location.accelerate_by(accel)
And two:
def reset(self):
self.position = u.CENTER
self.velocity = Vector2(0, 0)
self._angle = 0
That can be:
def reset(self):
self.move_to(u.CENTER)
self.accelerate_to(Vector2(0, 0))
self._angle = 0
With move_to and accelerate_to just forwarding to _location.
Now the only calls to the velocity setter are tests, so I rename it:
@velocity_testing_only.setter
def velocity_testing_only(self, velocity):
self._location.velocity = velocity
Now for position, they don’t seem to offer improvement. I think we have settled in on the notion that, so far, you can access a flyer’s position and not its velocity. So that’s nice.
Commit: improve privacy in Ship.
Summary
We’ve made small style improvements, marking some members “private”. I think that’s going to be a good practice to follow: it makes it just a bit more clear when we look at a class whether any of its variables are accessed from outside.
I wonder: would it be a useful thing to mark member functions with underbar, not just variables? Might that make reading the code easier because we could more readily see what’s just internal? Must think about that.
I don’t think I’d go so far as to mark things private and then make accessors. In a lower-trust team environment, that might be a good practice, or if we were publishing a library, but here we are protecting ourselves more against mistakes, so we don’t need serious protection. Probably.
As I work on other classes, I’ll make similar name changes. I’ll mention them if they seem significant, otherwise you’ll just see them turning up in the code that we look at.
During this exercise, we found a couple of methods that could be removed. Not a big savings but it’s always good to remove code that isn’t used.
I’ll break here. See you next time!