Python 209 - Responsibilities Emerge
Python Asteroids+Invaders on GitHub
There may be room for more refactoring in the fleet/group area. What even are those two objects? New CSS, consider clearing browser caches.
Stu Cox asked me about the responsibilities of InvaderFleet vs InvaderGroup. He felt that the group might wind up absorbing all the responsibilities of the fleet. I think Stu would have considered the various responsibilities first and then begun to separate them out.
I said in my reply:
I made the step to see what happened with the smart collection. It wasn’t a step /toward/ any particular goal, just a step that I usually find useful.
I agree that we can feel everything in the fleet falling in there, which of course wouldn’t be great. We’ll see,
And that’s the truth. I made the move to a smarter collection just because I generally find that wrapping collections is a good thing. Let’s guess at a division of responsibilities that might make sense.
Fleet: Overall Logic
Possibly the Fleet is there to manage the overall logic of the invaders as a group, moving the group as a whole back and forth and down. Possibly the Group manages individual moves. Probably the invaders manage their own position relative to the Fleet and their current form, one of the two shapes each invader has.
Group: Individual Logic
Probably individual invaders will report whether they are colliding with a player bullet. They might mark themselves as absent / dead, or, possibly, the Group will remove them from the collection entirely. Either way there will be issues around handling missing invaders, in the timing and moving area.
These are guesses. I imagine that with six decades of experience, I could sit down and do a paper design and get it nearly right, with different objects pretty well situated to divide up the things that we have to do. But that is not the style I choose, and for what I think are good reasons.
1: We’re usually in the middle.
For most programmers, most of the time, we are in the middle of our program’s life, not at its beginning. So we have less opportunity to do grand design, although we might do a bit of local design when time permits. But all too often, we have little time for “speculation”, so after a few scribbles on the whiteboard and a chat with our teammates, we have to get to it.
2: The middle gets messy.
When we, or our predecessors, have worked that way for a while, the program gets messy. In my view, we do best when we have decent tests, and when, discovering a mess, we make fairly local changes to improve the situation.
3: I simulate “the middle”.
So I am simulating that kind of experience with my little programs here. That said, I am doing other things as well, including picking programs and problems that I find interesting, and that are of a scale that I can work with as an individual. But, mostly, I’m working with incremental improvement, because for many of us programmers, much of the time, that’s all we can do.
4: And, well, I really do enjoy improving the code.
Finally, I should freely grant that I enjoy taking code that isn’t great and making it better. I enjoy pushing things this way and that, responding to the forces that I notice, making changes toward better. Generally, a few of those changes do make things better. Sometimes, I begin to see a goal for the code. Sometimes, moving toward that goal, or even after getting there, I don’t like it.
Analogy? Maundering? You decide.
When we used to take long vacation drives, we might identify some interesting places to visit along our route to my brother’s house or wherever we were really going. And we’d take a guess at the town we’d stay in. But sometimes we’d notice something that wasn’t on our list. The Shelby Gem Factory, for example. Visiting that would take some time and it would change where we’d be when we got tired. So we’d say, OK, we’ll stop in the next town. Often, we did just that. Sometimes that town seemed ratty. We might press on, or even turn back to a previous motel that we had noticed.
We would find our way, tending west, but finding good things, and not so good things, along the way. Those vacations were generally quite good, with many stories to tell. I like it when programming is like that, exploring, finding things, sometimes keeping them, and sometimes not.
Invaders?
Oh, right. Where was I? Invaders. Right.
Well, I’m not sure where the Fleet / Group idea will go. One thing we were thinking about was the rate of change of the member fields in InvaderFleet:
class InvaderFleet(Flyer):
def __init__(self):
self.step = Vector2(8, 0)
self.down_step = Vector2(0, 32)
self.invader_group = InvaderGroup()
self.origin = Vector2(u.SCREEN_SIZE / 2 - 5*64, 512)
self.invader_group.position_all_invaders(self.origin)
self.reverse = False
self.direction = 1
self.next_invader = len(self.invader_group.invaders)
step and down_step are basically constants, and probably should be programmed as constants. The origin varies on each cycle, and, so far, the invader_group just sticks around, though we know that it will be changing inside. reverse and direction change every N cycles, as the Fleet bounces back and forth between the bumpers. And next_invader changes on every update.
It seems to me that the next_invader notion should be pushed down to the Group. Let’s see how we use next_invader:
def update(self, delta_time, _fleets):
self.check_end_cycle(delta_time)
self.invader_group.set_invader_position(self.next_invader, self.origin)
self.next_invader += 1
def check_end_cycle(self, delta_time):
if self.next_invader >= len(self.invader_group.invaders):
self.reverse_or_continue(delta_time)
def reverse_or_continue(self, delta_time):
# we use +, not += because += modifies in place.
if self.reverse:
self.reverse = False
self.direction = -self.direction
self.origin = self.origin + self.direction * self.step + self.down_step
else:
self.origin = self.origin + self.direction * self.step
self.next_invader = 0
def at_edge(self, bumper_incoming_direction):
self.reverse = bumper_incoming_direction == self.direction
What if we were to push the next_invader notion down into Group, so that update would just send something like update_next(self.origin). And what if that method answered with a True / False, or maybe called us back, telling us when it has just moved the last invader?
What might it look like if InvaderGroup were more helpful to InvaderFleet? I’ll try sketching some code, not typing this in — yet.
class InvaderFleet(Flyer):
def update(...):
result = self.invader_group.update_next(self.origin)
if result == "end_cycle":
if self.reverse:
self.reverse_travel()
else
self.step_origin()
As I sketched that, I was “deciding” that when the Group moves its last invader, it just wraps around to the first, trusting that it’ll get the right origin next time around.
I’m leaving open the possibility of returning some other values that we could check. Certainly there must be one more, perhaps "ok" or something like that, that we don’t even check in the sketch here.
That seems interesting enough to try it. Let’s see if we can test-drive update_next in the Group.
def test_update_next(self):
group = InvaderGroup()
origin = Vector2(100, 100)
for i in range(54):
result = group.update_next(origin)
assert result == "ok"
result = group.update_next(origin)
assert result == "end"
That might be what I have in mind: the last call to next returns “end”, the rest return “ok”.
We need to make the group understand this.
Note, Stu and others, what thought steps have taken place so far:
- Notice that next_invader is moving 55 times faster than the other variables;
- Think it needs pushing down. The Group exists and seems a possibility;
- Sketch how the call might work;
- Find it tasty;
- Test-drive it in (we hope).
This isn’t random programming, far from it. It’s thoughtful, but unquestionably a bit intuitive. I’m not consciously applying deep design principles and rules, although I do know a lot of those and they form my intuition. To me, it seems mostly that I’m applying very simple guidelines, repeatedly.
What guidelines? Eliminating duplication. Breaking large things into small things. Telling objects to do things rather than asking them for information.
Here’s the new method in InvaderGroup:
class InvaderGroup:
def update_next(self, origin):
invader = self.invaders[self.next_invader]
invader.set_position(origin)
self.next_invader += 1
if self.next_invader < len(self.invaders):
return "ok"
else:
self.next_invader = 0
return "end"
Test passes. Let’s see if we can use it in InvaderFleet. Reviewing, InvaderFleet looks like this:
class InvaderFleet(Flyer):
def update(self, delta_time, _fleets):
self.check_end_cycle(delta_time)
self.invader_group.set_invader_position(self.next_invader, self.origin)
self.next_invader += 1
def check_end_cycle(self, delta_time):
if self.next_invader >= len(self.invader_group.invaders):
self.reverse_or_continue(delta_time)
def reverse_or_continue(self, delta_time):
# we use +, not += because += modifies in place.
if self.reverse:
self.reverse = False
self.direction = -self.direction
self.origin = self.origin + self.direction * self.step + self.down_step
else:
self.origin = self.origin + self.direction * self.step
self.next_invader = 0
Let’s just put in roughly what I sketched:
def update(self, delta_time, _fleets):
result = self.invader_group.update_next(self.origin)
if result == "end_cycle":
if self.reverse:
self.reverse_travel()
else:
self.step_origin()
def step_origin(self):
self.origin = self.origin + self.direction * self.step
def reverse_travel(self):
self.reverse = False
self.direction = -self.direction
self.origin = self.origin + self.direction * self.step + self.down_step
I just extracted the new step_origin and reverse_travel methods from the existing methods. You can see them in the old code above.
A test is failing. It thinks nothing has moved. I run the game. Nothing moves. Oh. I am checking for “end_cycle” and the keyword is “end”.
def update(self, delta_time, _fleets):
result = self.invader_group.update_next(self.origin)
if result == "ok":
pass
elif result == "end":
if self.reverse:
self.reverse_travel()
else:
self.step_origin()
else:
assert False
It just seemed prudent to check for an unknown return, since I’ve already had one. I’ll look up how to throw an exception when I get a moment.
The above nearly works. I say nearly, because the first cycle through, we use the existing origin, before incrementing it, so we stand still for one second. That’s what’s breaking the test, too, I think. Therefore:
class InvaderFleet(Flyer):
def __init__(self):
self.step = Vector2(8, 0)
self.down_step = Vector2(0, 32)
self.invader_group = InvaderGroup()
self.origin = Vector2(u.SCREEN_SIZE / 2 - 5*64, 512)
self.invader_group.position_all_invaders(self.origin)
self.reverse = False
self.direction = 1
self.step_origin()
Game looks good, but now two tests are failing. Let’s see.
def test_fleet_origin_is_centered(self):
fleet = InvaderFleet()
assert fleet.origin == Vector2(u.SCREEN_SIZE / 2 - 5*64, 512)
invader = fleet.testing_only_invaders[5] # bottom row middle column
assert invader.position.x == 512
This is now failing:
Expected :<Vector2(192, 512)>
Actual :<Vector2(200, 512)>
That’s because we have stepped the origin. Fix the first assert:
def test_fleet_origin_is_centered(self):
fleet = InvaderFleet()
assert fleet.origin == Vector2(u.SCREEN_SIZE / 2 - 5*64, 512) + Vector2(8, 0)
invader = fleet.testing_only_invaders[5] # bottom row middle column
assert invader.position.x == 512
The other test:
def test_fleet_motion(self):
fleet = InvaderFleet()
fleet.step = Vector2(30, 0)
pos = fleet.testing_only_invaders[0].position
fleet.update(1.0, None)
new_pos = fleet.testing_only_invaders[0].position
assert fleet.direction == +1
assert new_pos - pos == fleet.step
fleet.at_edge(+1)
assert fleet.reverse
fleet.next_invader = len(fleet.testing_only_invaders)
fleet.update(1.0, None)
assert fleet.direction == -1
Failure is:
> assert new_pos - pos == fleet.step
E assert <Vector2(8, 0)> == <Vector2(30, 0)>
E Full diff:
E - <Vector2(30, 0)>
E ? ^^
E + <Vector2(8, 0)>
E ? ^
We can’t really check that any more, because the Fleet has already used the real step. What if we don’t set it? We get further but not to completion.
fleet.at_edge(+1)
assert fleet.reverse
fleet.next_invader = len(fleet.testing_only_invaders)
fleet.update(1.0, None)
> assert fleet.direction == -1
E assert 1 == -1
E + where 1 = <invaderfleet.InvaderFleet object at 0x105f84910>.direction
This test knows too much about how things move. Let’s see if we can suss out its meaning and make sure that gets checked somewhere.
This test checks at least two situations. First it is checking that the fleet moves invaders. I guess that’s OK, and it runs now. The group does the work but we are at least checking, via the call to fleet.update, that the fleet asks the group to do it. We could, in principle, fail to do that.
Then it’s trying to check reverse, which is correctly set if you call at_edge. So that’s legit.
But then it wants to know if the fleet has updated the direction, but we really only do that now when the Group has reported we’re at the end.
How might we test that? First, I’ll remove the last bits from the test we’re working on. That will get us to green.
def test_fleet_motion(self):
fleet = InvaderFleet()
# fleet.step = Vector2(30, 0)
pos = fleet.testing_only_invaders[0].position
fleet.update(1.0, None)
new_pos = fleet.testing_only_invaders[0].position
assert fleet.direction == +1
assert new_pos - pos == fleet.step
fleet.at_edge(+1)
assert fleet.reverse
# fleet.next_invader = len(fleet.testing_only_invaders)
# fleet.update(1.0, None)
# assert fleet.direction == -1
We’re actually good now. Let’s commit, it has been all morning, and that’s not good.
Commit: InvaderGroup now handles updating individual invaders, returning “ok” normally,and “end” when at the end of the group, so that InvaderFleet knows to move the origin.
Now we’d like to be sure that when we get at_end, and when the group is done moving, we will reverse the direction flag. Or, maybe we’d really like to see that the step has reversed its x direction.
We actually have four tests that are aimed at this question, and every single one of them is wrong (pace Kipling):
def test_direction_reverses_at_edge(self):
fleet = InvaderFleet()
assert fleet.direction == +1
fleet.at_edge(+1)
fleet.reverse_or_continue(1.0)
assert fleet.direction == -1
def test_direction_unchanged_when_not_at_edge(self):
fleet = InvaderFleet()
assert fleet.direction == +1
fleet.reverse_or_continue(1.0)
assert fleet.direction == +1
def test_step_down_at_edge(self):
fleet = InvaderFleet()
origin = fleet.origin
fleet.at_edge(+1)
fleet.reverse_or_continue(1.0)
assert fleet.origin == origin - fleet.step + fleet.down_step, \
"if this fails someone may have modified a vector in place"
def test_no_step_down_when_not_at_edge(self):
fleet = InvaderFleet()
origin = fleet.origin
fleet.reverse_or_continue(1.0)
assert fleet.origin == origin + fleet.step, \
"if this fails someone may have modified a vector in place"
That reverse_or_continue method should not be used. It needs removal as do one other:
class InvaderFleet(Flyer):
def check_end_cycle(self, delta_time):
if self.next_invader >= len(self.invader_group.invaders):
self.reverse_or_continue(delta_time)
def reverse_or_continue(self, delta_time):
# we use +, not += because += modifies in place.
if self.reverse:
self.reverse_travel()
else:
self.step_origin()
self.next_invader = 0
I remove those. My four tests fail for lack of anyone to talk to.
Let’s refresh my mind on the new logic:
def update(self, delta_time, _fleets):
result = self.invader_group.update_next(self.origin)
if result == "ok":
pass
elif result == "end":
if self.reverse:
self.reverse_travel()
else:
self.step_origin()
else:
assert False
def step_origin(self):
self.origin = self.origin + self.direction * self.step
def reverse_travel(self):
self.reverse = False
self.direction = -self.direction
self.origin = self.origin + self.direction * self.step + self.down_step
What we should be concerned with is testing that if nest in update. Let’s extract it, which will make it testable without involving the group.
def update(self, delta_time, _fleets):
result = self.invader_group.update_next(self.origin)
self.process_result(result)
def process_result(self, result):
if result == "ok":
pass
elif result == "end":
if self.reverse:
self.reverse_travel()
else:
self.step_origin()
else:
assert False
Now we can test that processing. If we were mockists, we’d test to see that we cal reverse_travel and step_origin, but we are not, so we will check the results. I’ll write new tests and I think three will replace the old four. We’ll see.
def test_ok_leaves_step_alone(self):
fleet = InvaderFleet()
origin = fleet.origin
fleet.process_result("ok")
assert fleet.origin == origin
Green. Then:
def test_end_increments_step(self):
fleet = InvaderFleet()
origin = fleet.origin
fleet.process_result("end")
assert fleet.origin == origin + fleet.step
Also green. And:
def test_end_at_edge_steps_down_and_left(self):
fleet = InvaderFleet()
origin = fleet.origin
direction = fleet.direction
fleet.at_edge(+1)
fleet.process_result("end")
assert fleet.direction == -direction
assert fleet.origin == origin - fleet.step + fleet.down_step
And green. Commit: new tests for “ok” and “end” and edge behavior.
Let’s sum up.
Summary
We pushed the moving of individual invaders down to the group, leaving moving the whole group up to the fleet. In so doing, we created a simple return convention from the update_next function, indicating an “ok” return, where nothing exceptional has happened, and an “end” return, signifying that we have updated the last invader. I suspect that in due time, we’ll have another return signifying that there are no invaders left in the group, at which point … I bet we’ll create another group. That remains to be seen. We can’t even kill a single invader yet.
So the responsibilities are becoming clearer, handling of individuals in the collection down in group, and handling the overall positioning of the collection and the general cycle logic, in the Fleet.
Notice the “and”. The fleet includes the logic to forward all the general game cycle calls, update, draw, and relevant collision events, and handling overall positioning. There’s a bit of a cohesion problem just sticking its head up, and we will try to keep that in mind.
What might we do? We might do nothing. We might think about pushing it down to the Group, but then the group is incohesive, handling group and individual things. So, if we do something, we might create some kind of small position handler object. For now, we choose to do nothing.
InvaderFleet is simpler, in that some logic has been moved, and the remaining logic has changed from a status kind of check to a simple response to a result from the group. The group now has a larger responsibility. It always handled the final step of updating a single invader, but now it also has knowledge of which invader is next.
You know what that reminds me of? It reminds me of an “iterator”, an object that manages iteration. Perhaps we’d like to have an iterator to help us with this work. That might be another object that is asking to be born.
We’ll wait and see. For now, we have better tests, the code is more clear, and the responsibilities are beginning to appear.
This is how I like it. I do all the thinking I’d do anyway, but I do it incrementally, and much of it is expressed immediately in code — code that gets better and better almost always. When it doesn’t get better — I make it better one way or another.
This is the way!
See you next time!