P-271 - Duplication
Python Asteroids+Invaders on GitHub
There’s some duplication between the BottomLine and Shield. Enough to fix? Let’s have a look. Outcome: This goes ever so nicely! Sweet!
Duplication? You decide:
class BottomLine(InvadersFlyer):
def __init__(self):
w = 960-128
h = 4
surface = Surface((w, h))
surface.set_colorkey((0, 0, 0))
surface.fill("green")
rect = Rect(0, 0, w, h)
rect.bottomleft = (64, u.SCREEN_SIZE - 56)
self.position = rect.center
self._map = surface
self._mask = pygame.mask.from_surface(surface)
self._rect = rect
self._tasks = Tasks()
@property
def mask(self):
return self._mask
@property
def rect(self):
return self._rect
def draw(self, screen):
screen.blit(self._map, self.rect)
def begin_interactions(self, fleets):
self._tasks.clear()
def end_interactions(self, fleets):
self._tasks.finish()
def interact_with(self, other, fleets):
other.interact_with_bottomline(self, fleets)
def interact_with_invadershot(self, shot, fleets):
self.process_shot_collision(shot)
def process_shot_collision(self, shot):
if Collider(self, shot).colliding():
self._tasks.remind_me(lambda: self.mash_image(shot))
def mash_image(self, shot):
masher = ImageMasher.from_flyers(self, shot)
self._mask, self._map = masher.update(self._mask, self._map)
class Shield(InvadersFlyer):
def __init__(self, position):
map = BitmapMaker.instance().shield
self._invader_shot_explosion = BitmapMaker.instance().invader_shot_explosion
self._invader_explosion_mask = pygame.mask.from_surface(self._invader_shot_explosion)
self._player_shot_explosion = BitmapMaker.instance().player_shot_explosion
self._player_explosion_mask = pygame.mask.from_surface(self._player_shot_explosion)
self._map = map.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(map)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
@property
def mask(self):
return self._mask
@property
def rect(self):
return self._rect
@property
def position(self):
return self._rect.center
def draw(self, screen):
screen.blit(self._map, self.rect)
def begin_interactions(self, fleets):
self._tasks.clear()
def end_interactions(self, fleets):
self._tasks.finish()
def interact_with(self, other, fleets):
other.interact_with_shield(self, fleets)
def interact_with_invadershot(self, shot, fleets):
self.process_shot_collision(shot, self._invader_shot_explosion, self._invader_explosion_mask)
def interact_with_playershot(self, shot, fleets):
self.process_shot_collision(shot, self._player_shot_explosion, self._player_explosion_mask)
def process_shot_collision(self, shot, explosion, explosion_mask):
collider = Collider(self, shot)
if collider.colliding():
self._tasks.remind_me(lambda: self.mash_image(shot))
def mash_image(self, shot):
masher = ImageMasher.from_flyers(self, shot)
self._mask, self._map = masher.update(self._mask, self._map)
If we were to inline that temp in process_shot_collision …
def process_shot_collision(self, shot, explosion, explosion_mask):
if Collider(self, shot).colliding():
self._tasks.remind_me(lambda: self.mash_image(shot))
It seems fair to say that these two classes are more than a little similar. If we recognize that some of the collisions for the one are impossible for the other, I think we could make them identical except for the __init__ … and their interact_with methods. But if they were the same class, there’d only be a need for one interact_with.
I think this is worth doing. Let’s move toward one class.
“Plan”
My rough plan is this:
-
Move toward one class, probably called RoadFurniture in honor of the Tour de France, where I learned that term for the stuff in the road that the bikers mostly hate.
-
Have a unique class method constructor for Shield and BottomLine, setting up the necessary objects.
-
Begin with the easy constructor, the BottomLine, on its own class.
-
After that works, write the Shield constructor as a class method on the BottomLine class.
-
Rename to RoadFurniture or whatever we call it.
No. Better to start with Shield, since it has more methods. Other than that, similar plan. I’m glad we had this little chat, I think I’d have started with BottomLine and had a bit more trouble if I had. Nothing fatal, just a pain.
Discovery
How do we create Shields now, and how often? Half a dozen places, all but one in tests. Let’s posit a new Shield constructor, shield, and code it up. What needs to move? We’ll take a closer look at the two inits:
class Shield(InvadersFlyer):
def __init__(self, position):
map = BitmapMaker.instance().shield
self._invader_shot_explosion = BitmapMaker.instance().invader_shot_explosion
self._invader_explosion_mask = pygame.mask.from_surface(self._invader_shot_explosion)
self._player_shot_explosion = BitmapMaker.instance().player_shot_explosion
self._player_explosion_mask = pygame.mask.from_surface(self._player_shot_explosion)
self._map = map.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(map)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
class BottomLine(InvadersFlyer):
def __init__(self):
w = 960-128
h = 4
surface = Surface((w, h))
surface.set_colorkey((0, 0, 0))
surface.fill("green")
rect = Rect(0, 0, w, h)
rect.bottomleft = (64, u.SCREEN_SIZE - 56)
self.position = rect.center
self._map = surface
self._mask = pygame.mask.from_surface(surface)
self._rect = rect
self._tasks = Tasks()
Plan vs Reality: Reality wins.
On closer examination, there are quite a few differences in here. When I look to see what the explosions and masks are about, I find that they are not actually used:
class Shield(InvadersFlyer):
def interact_with_invadershot(self, shot, fleets):
self.process_shot_collision(shot, self._invader_shot_explosion, self._invader_explosion_mask)
def interact_with_playershot(self, shot, fleets):
self.process_shot_collision(shot, self._player_shot_explosion, self._player_explosion_mask)
def process_shot_collision(self, shot, explosion, explosion_mask):
if Collider(self, shot).colliding():
self._tasks.remind_me(lambda: self.mash_image(shot))
- Note
- What I’m doing here is this: Noting that these two classes aren’t quite as identical as they might be, I’m going to refactor them to be more similar before I start with the combining. These small initial steps will carve off bits of difference that would have to be dealt with later, and when we have two “classes” using the same actual class, we want as few differences as possible, ideally all in the constructors.
Change Signature on process_shot_collision to remove the unused parameters.
def interact_with_invadershot(self, shot, fleets):
self.process_shot_collision(shot)
def interact_with_playershot(self, shot, fleets):
self.process_shot_collision(shot)
def process_shot_collision(self, shot):
if Collider(self, shot).colliding():
self._tasks.remind_me(lambda: self.mash_image(shot))
No fool this morning, at least not yet, I test that in the game. We’re good. Now I can remove those babies from the init:
class Shield(InvadersFlyer):
def __init__(self, position):
map = BitmapMaker.instance().shield
self._map = map.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(map)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
Compared with:
class BottomLine(InvadersFlyer):
def __init__(self):
w = 960-128
h = 4
surface = Surface((w, h))
surface.set_colorkey((0, 0, 0))
surface.fill("green")
rect = Rect(0, 0, w, h)
rect.bottomleft = (64, u.SCREEN_SIZE - 56)
self.position = rect.center
self._map = surface
self._mask = pygame.mask.from_surface(surface)
self._rect = rect
self._tasks = Tasks()
I note the odd treatment of position and the rectangle center. The difference is that Shield does this:
@property
def position(self):
return self._rect.center
Let’s change BottomLine this time, to match up with Shield.
class BottomLine(InvadersFlyer):
def __init__(self):
w = 960-128
h = 4
surface = Surface((w, h))
surface.set_colorkey((0, 0, 0))
surface.fill("green")
rect = Rect(0, 0, w, h)
rect.bottomleft = (64, u.SCREEN_SIZE - 56)
self._map = surface
self._mask = pygame.mask.from_surface(surface)
self._rect = rect
self._tasks = Tasks()
@property
def position(self):
return self._rect.center
Eek, I should be committing. Commit: Refactoring to make BottomLine and Shield more similar.
OK, how do the inits look now:
class Shield(InvadersFlyer):
def __init__(self, position):
map = BitmapMaker.instance().shield
self._map = map.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(map)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
Let’s reorder the lines. I want them to look as similar as possible. Less thinking required that way. Here’s the next step:
class BottomLine(InvadersFlyer):
def __init__(self):
w = 960-128
h = 4
surface = Surface((w, h))
surface.fill("green")
rect = Rect(0, 0, w, h)
rect.bottomleft = (64, u.SCREEN_SIZE - 56)
self._map = surface
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(surface)
self._rect = rect
self._tasks = Tasks()
class Shield(InvadersFlyer):
def __init__(self, position):
map = BitmapMaker.instance().shield
self._map = map.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(map)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
Commit: Refactoring for similarity.
Rename map to surface:
class Shield(InvadersFlyer):
def __init__(self, position):
surface = BitmapMaker.instance().shield
self._map = surface.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(surface)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
class BottomLine(InvadersFlyer):
def __init__(self):
w = 960-128
h = 4
surface = Surface((w, h))
surface.fill("green")
rect = Rect(0, 0, w, h)
rect.bottomleft = (64, u.SCREEN_SIZE - 56)
self._map = surface
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(surface)
self._rect = rect
self._tasks = Tasks()
Commit, same message.
Can I change BottomLine to get the rect and set the center the same way that Shield does?
I think I can, but let’s test that.
def test_rect(self):
w = 960-128
h = 4
surface = Surface((w, h))
rect = Rect(0, 0, w, h)
assert surface.get_rect() == rect
Yes, but there’s something going on that I don’t quite see. Look again, there are odd things going on with the rects and position and center:
class BottomLine(InvadersFlyer):
def __init__(self):
w = 960-128
h = 4
surface = Surface((w, h))
surface.fill("green")
rect = surface.get_rect() # <===
rect.bottomleft = (64, u.SCREEN_SIZE - 56) # <===
self._map = surface
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(surface)
self._rect = rect # <===
self._tasks = Tasks()
class Shield(InvadersFlyer):
def __init__(self, position):
surface = BitmapMaker.instance().shield
self._map = surface.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(surface)
self._rect = self._map.get_rect() # <===
self._rect.center = position # <===
self._tasks = Tasks()
Ah. I need position as the rect center.
class BottomLine(InvadersFlyer):
def __init__(self):
w = 960-128
h = 4
surface = Surface((w, h))
surface.fill("green")
rect = surface.get_rect()
rect.bottomleft = (64, u.SCREEN_SIZE - 56)
position = rect.center
self._map = surface
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(surface)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
class Shield(InvadersFlyer):
def __init__(self, position):
surface = BitmapMaker.instance().shield
self._map = surface.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(surface)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
Add the copy and they’ll be the same below the blank line:
class BottomLine(InvadersFlyer):
def __init__(self):
w = 960-128
h = 4
surface = Surface((w, h))
surface.fill("green")
rect = surface.get_rect()
rect.bottomleft = (64, u.SCREEN_SIZE - 56)
position = rect.center
self._map = surface.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(surface)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
class Shield(InvadersFlyer):
def __init__(self, position):
surface = BitmapMaker.instance().shield
self._map = surface.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(surface)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
Commit: Refactoring for similarity.
Now I think we’re good. The final six lines both take the same two values from above, surface and position. We have identified the parameters we need for a common init!
Let’s do the Shield constructor:
class Shield(InvadersFlyer):
@classmethod
def shield(cls, position):
surface = BitmapMaker.instance().shield
return cls(surface, position)
def __init__(self, surface, position):
self._map = surface.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(surface)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
With the suitable changes to class users, we’re good. For example:
coin.py
def invaders(fleets):
fleets.clear()
...
for i in range(3):
fleets.append(ReservePlayer(i))
half_width = 88 / 2
spacing = 198
step = 180
for i in range(4):
place = Vector2(half_width + spacing + i*step, 800-16)
fleets.append(Shield.shield(place))
Commit: Shield uses new constructor shield.
Now, it seems to me, we can do a constructor for the BottomLine, in Shield class.
I wonder what will happen at run time. It should just work.
class Shield(InvadersFlyer):
@classmethod
def shield(cls, position):
surface = BitmapMaker.instance().shield
return cls(surface, position)
@classmethod
def bottom_line(cls):
w = 960-128
h = 4
surface = Surface((w, h))
surface.fill("green")
rect = surface.get_rect()
rect.bottomleft = (64, u.SCREEN_SIZE - 56)
position = rect.center
return cls(surface, position)
And in use:
def invaders(fleets):
fleets.clear()
left_bumper = 64
fleets.append(Bumper(left_bumper, -1))
fleets.append(Bumper(960, +1))
fleets.append(TopBumper())
fleets.append(InvaderFleet())
fleets.append(PlayerMaker())
fleets.append(ShotController())
fleets.append(InvaderScoreKeeper())
fleets.append(Shield.bottom_line()) # <===
for i in range(3):
fleets.append(ReservePlayer(i))
half_width = 88 / 2
spacing = 198
step = 180
for i in range(4):
place = Vector2(half_width + spacing + i*step, 800-16)
fleets.append(Shield.shield(place)) # <===
We are good. Commit: BottomLine now uses Shield class.
Now we can remove BottomLine class and its nearly useless tests.
Two uses of PyCharm Safe Delete and we are good to go. Commit: Remove BottomLine class and tests.
Rename the Shield class to RoadFurniture. PyCharm renames the file but not the required method in InvaderFlyer … and we’ll need to rename references as well.
Renaming interact_with_shield to interact_with_roadfurniture gets us green.
Commit: rename Shield to RoadFurniture. Corresponding interactions changed.
Summary
So that was interesting, wasn’t it? Eight commits in less than an hour. Nothing ever broke, though I did make a false start and roll back once that I didn’t mention. Sort of thing you’d just Command+Z out of and do again. It was right after a commit, so rollback was safer and easier.
The process consisted of step after step to make the two init methods more and more alike, until we have six identical lines preceded by different setup. Then, in essence, we extracted the two different bits to two class methods, and voila! both shield and bottom line were both using shield class. Remove the unused class and tests, rename the common class.
Each step was so small and simple that I just about couldn’t do it wrong. Sure, I probably could have done it in four steps, but they’d have been difficult, tricky, and very likely I’d have made a mistake.
I wouldn’t call these steps mindless: they were each carefully selected to produce duplication, making the two methods more and more alike. But each one was just a line or two of change. And committing after each one ensured that if I did take a misstep, I could get right back to a good spot.
Subjectively, the process felt very calm and peaceful. Note a difference, think how to make it no longer different. Move lines, change one to be like the other, tiny steps.
That went very nicely. Many More much Smaller Steps. Thanks, Hill!
See you next time!