Python 038 - Technical Stories? No
I may have casually been referring to the yellow sticky notes in my keyboard tray Jira as “stories”. That’s not consistent with my classic advice about stories. And my first-ever pull request!
Woot! A Pull-Request
Sometimes people browse my GitHub repos, and I think at least once in the past someone actually cloned one for a bit of learning. But this week I got my first-ever pull request.
Rickard Lindberg tried out this program and got it running. He ran into the usual version issues, though I had been told by the people who tell me things that python’s venv thing avoids those. Anyway, Rickard didn’t have too much trouble.
He doesn’t use IDEs, so he wanted a couple of little scripts, one to run the tests and one for the program. And he issued a pull request to me, the first one ever in my whole life. (Not the first time anyone improved or fixed my code, but the first pull request). I’ll accept that request as soon as I figure out how.
I’m so delighted that someone out there looks at what I do, and surprisingly chuffed to get an actual pull request. And you should check out his site: I know that I will be.
Thanks Rickard! Woot!
Stories
One of the weaknesses of XP, Scrum, and similar approaches is the separation of technical decisions making from business-oriented decision-making. This is also a strength of those approaches, in particular because it makes it more clear where the responsibility lies to have the best possible product by the desired delivery date: squarely in the hands of the business people, not the technical people. You deliver the best possible product by selecting the most important things to do, in the light of the actual progress the team is making.
But the weakness is an important one as well, which is that these approaches can set up “sides”, where what we really need is enlightened collaboration among all the team members to create the best possible outcome. One common idea to bridge the gap is the “technical story”. The developers have a need to do something inside the code, that will have no visible effect on the product. Commonly, they need to refactor something to make new capabilities possible, or easier to build. They think this will take substantial time, and therefore they want to schedule it, and they want a “technical story” to go into the mix.
The problem with this idea should be obvious: the business-side person is not technical and cannot possibly assess the value of this story. They look at it, ask how it improves the product, see that it doesn’t have visible effect. They assign it zero value, which means that it will be scheduled for the day after never.
I’ve written about this in Refactoring – Not on the backlog! and in Technical Stories: We don’t need ‘em..
The upshot here is that, strictly speaking, notes in my Jira like “Saucer” are story notes, because they represent business value, a scary saucer that shoots you down, causing you to put more quarters into the machine. There’s yer biznis value raht there. But the note next to that one says “main loop”, meaning that the code in the main loop probably needs reorganization. That one is not a story, it’s a note from the developers (me) to themselves (also me) about something that they would like to work on.
Now, of course, I write about everything that I do, and so for me, “main loop” has business value in that I’ll get a refactoring-focused article out of it, so I can rationalize calling them all stories, but I don’t really do that. What does happen is that I am careless with my language and might refer to “main loop” as a story … even though I would not recommend to most teams that they treat such things as stories. As the articles above suggest, the team should just use their time effectively, which is to say that when they are working in an area that needs cleaning up or other improvement, they do it as part of the work. Since improvement in our actual work area makes the work go better, this generally averages out or actually speeds us up, so it is an effective way to work.
That’s my story1 and I’m sticking to it. Let’s get to work.
Today’s Activity
Yesterday I completed the task of getting all the top-level functions into a class, Game. I think there are no top-level functions left in the program, none at all. There may be a top-level helper function in the tests, but there’s a good chance that it’s embedded in a test class.
I do have some stories that I could work on, including “Saucer”, “Star Field”, and “Hyperspace”. But I don’t want to do those yet, because there are two issues that I think are more interesting for my coding and writing purposes.
One is that the game’s score is kept in a variable in the module u, where the constants reside. Everything else is in there is just a constant, like u.SPEED_OF_LIGHT But there is u.score, a value that is used to display the score. And that score is tallied here:
class Asteroid ...
def destroyed_by(self, attacker, asteroids):
u.score += attacker.score_list[self.size]
self.split_or_die(asteroids)
destroyed_by is called from here:
class Game ...
def mutual_destruction(self, target, targets, attacker, attackers):
if self.within_range(target, attacker):
attacker.destroyed_by(target, attackers)
target.destroyed_by(attacker, targets)
I’d like to get rid of that globalist cross-module reference to a writable number. So that’s one issue, and it is entangled with the other, larger issue.
The Game class is clearly too broad. Look at its member variables:
def __init__(self, testing=False):
self.asteroids = []
self.asteroids_in_this_wave = None
self.delta_time = 0
self.game_over = False
self.game_over_pos = None
self.game_over_surface = None
self.help_lines = None
self.missiles = []
self.running = False
self.score_font = None
self.ship = Ship(pygame.Vector2(u.SCREEN_SIZE / 2, u.SCREEN_SIZE / 2))
self.ship_timer = 0
self.ships = []
self.ships_remaining = 0
self.wave_timer = u.ASTEROID_TIMER_STOPPED
if not testing:
self.clock = pygame.time.Clock()
self.screen = pygame.display.set_mode((u.SCREEN_SIZE, u.SCREEN_SIZE))
OK, it’s a top level object, so we shouldn’t be surprised at some breadth. It has the main loop in it, and that implies that, in an important sense, Game does everything. But it does everything all by itself, except for the very rudimentary objects asteroid, missile, saucer, and ship, and little else. It needs to be refactored.
Here are some of Game’s responsibilities:
- set up a new game
- define all the surfaces
- check and dispatch keyboard events
- check all possible collisions
- move all the objects
- draw all the objects
For some of these things, details are deferred to the objects, but the details are often known to Game.
But isn’t this OK, in a tiny little program like this?
In a little program like this one, yes, sure, it might be OK. But while we only work on small programs for these articles, we think big-program thoughts, because almost all the changes we need to make to a big program appear in our small programs. So an issue that we might live with here is one that in a larger program we would be better off not resolve. In fact, from some angles, resolving the issues here will make the program “better”.
Back to u.score
An easy change for u.score would be to make it self.score in Game, and add a new method, something like score_against(attacker) that would return the score, and then call that method from the mutual_destruction method, tuck it away, and pass it to the score display code.
But. But … we should probably defer collision-handling to a separate object. And perhaps drawing. And if we do that, then we’ll have to figure out a way to get score back from that deferred object. OK, just telling you about it has made me less worried. Presumably we’ll call the Collider and it can return the score update value. It’ll be OK, Ron, calm down.
OK, that has convinced me to start with u.score. One hundred ten lines and I’m ready to write about three lines of code.
Is that bad? No. Thinking is good, and in fact it netted me some confidence. I’m glad we had this little chat, and wish it could have been live instead of written down. And I wish you weren’t imaginary.
There are thirteen references to u.score, five in game code and eight in tests. We’ll just do it, first the code then the tests. No, let’s do the tests first, just to see what we can do in advance. The basic plan is that Game will have a new member, score, replacing u.score.
def test_asteroid_saucer_does_not_score(self):
game = Game(True)
u.score = 0
pos = Vector2(100, 100)
asteroid = Asteroid(2, pos)
print("position", asteroid.position)
asteroids = [asteroid]
saucer = Saucer(pos)
saucers = [saucer]
game.mutual_destruction(asteroid, asteroids, saucer, saucers)
assert not saucers
assert u.score == 0
The change is obvious: refer to game.score, not u.score. I make that change throughout the tests. Only one fails, because only one expects a non-zero value. A bit disconcerting, but I’ll think about it after I make this work.
In Game, I change them all to self.score, and PyCharm helps me remember to put it into the __init__. But we need to fix up mutual_destruction and Asteroid.
class Game ...
def mutual_destruction(self, target, targets, attacker, attackers):
if self.within_range(target, attacker):
self.score += target.score_against(attacker)
self.score += attacker.score_against(target)
attacker.destroyed_by(target, attackers)
target.destroyed_by(attacker, targets)
I just figure we’ll ask everyone and most of them will return zero.
class Asteroid
def destroyed_by(self, attacker, asteroids):
self.split_or_die(asteroids)
def score_against(self, attacker):
return attacker.score_list[self.size]
And the other three classes, I’ll just return zero.
def score_against(self, _):
return 0
When I put those in place, all the tests start passing. While I was in the middle of it, as many as four were failing. I didn’t really look at them: I was confident that when things were right, they’d all run.
Now u.score should be unreferenced. Remove it. Run the game to be sure the score actually works on the display. No surprise, it does. Commit: move u.score into game as self.score.
Reflection
What about all those tests that just check to see that the resulting score is zero? They were working even with u.score disconnected, because they init game.score to zero and then later check it. So they just jam a zero into the game instance and check it. Downside of Python. We can fix those up by removing the initial setting. Here’s an example:
def test_asteroid_saucer_does_not_score(self):
game = Game(True)
game.score = 0
pos = Vector2(100, 100)
asteroid = Asteroid(2, pos)
print("position", asteroid.position)
asteroids = [asteroid]
saucer = Saucer(pos)
saucers = [saucer]
game.mutual_destruction(asteroid, asteroids, saucer, saucers)
assert not saucers
assert game.score == 0
If we don’t init game.score, we can be sure that whatever we get is what the game served up, not our own zero back. So I’ll just remove all those lines and expect the tests to stay green.
That’s what happens. Now, when we mess up the refactoring, these tests have a shot at finding the problem. Commit: improve collision tests not to pre-init score.
Reflectoplan
OK, this went nicely and I think our tests are actually a tiny bit better. What shall we do next? This article is almost 200 lines now, and while it is only 0930, I don’t think I want to attack moving collisions out.
Let’s look at how objects are drawn.
Draw
class Game ...
def draw_everything(self):
screen = self.screen
screen.fill("midnightblue")
for ship in self.ships:
ship.draw(screen)
for asteroid in self.asteroids:
asteroid.draw(screen)
for missile in self.missiles:
missile.draw(screen)
self.draw_score()
self.draw_available_ships()
class Asteroid ...
def __init__(self, size=2, position=None):
self.size = size
if self.size not in [0,1,2]:
self.size = 2
self.radius = [16, 32, 64][self.size]
self.position = position if position is not None else Vector2(0, random.randrange(0, u.SCREEN_SIZE))
angle_of_travel = random.randint(0, 360)
self.velocity = u.ASTEROID_SPEED.rotate(angle_of_travel)
self.offset = Vector2(self.radius, self.radius)
self.surface = SurfaceMaker.asteroid_surface(self.radius * 2)
def draw(self, screen):
top_left_corner = self.position - self.offset
pygame.draw.circle(screen, "red", self.position, 3)
screen.blit(self.surface, top_left_corner)
OK, that’s better than it might have been. The space objects each create their own surface and blit it. The ship jumps through its own orifice to select a surface with or without the flare and to rotate it, but it’s all inside Ship class.
There is some surface creation going on for Score and the GAME OVER screen. That’s in game_init:
def game_init(self):
pygame.init()
self.screen = pygame.display.set_mode((u.SCREEN_SIZE, u.SCREEN_SIZE))
pygame.display.set_caption("Asteroids")
self.define_game_over()
self.define_score()
self.running = True
self.insert_quarter(0)
I suspect that the first five lines could be moved to the object init. Let’s try that. It works fine. Commit: Move some graphics init into Game.__init__.
I could imagine a score object and a GAME_OVER object, but that seems to me to have low value at the moment.
One look at the main loop, just to set our minds in motion for next time.
def main_loop(self):
self.game_init()
while self.running:
for event in pygame.event.get():
if event.type == pygame.QUIT:
self.running = False
self.check_ship_spawn(self.ship, self.ships, self.delta_time)
self.check_next_wave(self.delta_time)
self.control_ship(self.ship, self.delta_time)
for missile in self.missiles.copy():
missile.update(self.missiles, self.delta_time)
self.move_everything(self.delta_time)
self.check_collisions()
self.draw_everything()
if self.game_over: self.draw_game_over()
pygame.display.flip()
self.delta_time = self.clock.tick(60) / 1000
pygame.quit()
Let’s extract that loop over missiles into check_missile_timeout(), Explaining Method.
...
self.check_ship_spawn(self.ship, self.ships, self.delta_time)
self.check_next_wave(self.delta_time)
self.check_missile_timeout()
self.control_ship(self.ship, self.delta_time)
self.move_everything(self.delta_time)
self.check_collisions()
self.draw_everything()
...
Commit: Explaining Method, check_missile_timeout.
I could imagine a somewhat better structure for all this. Perhaps the major checks for spawning, and then a call to everyone to update, which would allow missiles to time out there, and would include everyone moving who wants to move. Then after we’re all in our places2, check collisions, then draw everyone, which will not include anyone recently destroyed3 and will include anyone recently added4.
We have a “main loop” technical note, so we’ll surely address something like that. For now, we’ve done enough, I think.
Summary
Some improvements to testing, small but potentially important. We’ve removed a remaining wart, that writable variable in u. We’ve made scoring a bit more explicit. We’ve improved the main loop a tiny bit, with a few keystrokes.
A good morning, more than I had expected for the day. See you next time!