Refactoring
Hello, loves!
Speed report, and I noticed what looks like Feature Envy. Let’s see where it leads. One serious mistake, quickly corrected. Very nice result. Some speculation.
With a little free time in hand while I attended a not very interesting meeting in another world, I sped up some tests, and made the scroller tests work without a window, so those are turned back on. The measured time for the tests is about 3/4 of a second. I found out that there is a fairly substantial delay creating a DungeonLayout of the standard size, 64x56, upwards of 3500 Cell instances. When I say substantial, I mean something between 0.01 and 0.02 seconds, so it’s not exactly a worry. Along the way I learned the incantations needed to convince pytest and PyCharm to display test timing. I haven’t checked whether doing so adds much time to the testing cycle but it seems not to.
Then I ran across some clear Feature Envy in Dungeon. Feature Envy is what we say about code that spends more time thinking about some other class than about its own concerns. Here’s Dungeon:
class Dungeon:
def attempt_move_to_offset(self, cell, offset):
new_cell = self.layout.dungeon_cell_at_offset(cell.xy, offset)
if new_cell and self.can_move_between(cell, new_cell):
self.place_player_at(new_cell)
def can_move_between(self, cell, new_cell):
return not self.in_different_rooms(cell, new_cell) or self.layout.has_passage(cell, new_cell)
def in_different_rooms(self, now, proposed):
return self.layout.get_room(now) != self.layout.get_room(proposed)
def place_player_at(self, cell):
interactor = Interactor(self, cell)
if all(interactor.execute(item) for item in list(self.contents_at(cell))):
self.player_cell = cell
When we look carefully at these methods, we see that we’re basically just asking the layout a series of questions that add up to “it’s OK to move from cell to new_cell”. Then we run the interactions and decide whether to move or not.
Now that we’re talking about it, maybe the right idea is “it’s OK to move from cell to the cell at offset”, since even the line that gets new_cell is all about layout.
A thing that PyCharm seems not to be able to do is move a method from one class to another without breaking anything. So we’re going to have to do this by hand, at least in part.
Refactoring Plan One
I think I’ll try in-lining the code up into attempt, and I suspect we’ll want to extract some variables so as to provide for an answer to come back. If this doesn’t work, we’ll create Plan Two.
First a variable:
def attempt_move_to_offset(self, cell, offset):
new_cell = self.layout.dungeon_cell_at_offset(cell.xy, offset)
can_move = new_cell and self.can_move_between(cell, new_cell)
if can_move:
self.place_player_at(new_cell)
Why? Because now, if all this were over in layout, the call to layout will replace the top two lines, assigning to can_move, and then we’ll probably inline it back.
Now inline can_move_between to see what we see.
def attempt_move_to_offset(self, cell, offset):
new_cell = self.layout.dungeon_cell_at_offset(cell.xy, offset)
can_move = new_cell and
not self.in_different_rooms(cell, new_cell)
or self.layout.has_passage(cell, new_cell)
if can_move:
self.place_player_at(new_cell)
Why? Because I want to get all the layout references into one blob that we can readily move.
Inline in_different_rooms:
def attempt_move_to_offset(self, cell, offset):
new_cell = self.layout.dungeon_cell_at_offset(cell.xy, offset)
can_move = new_cell and not self.layout.get_room(cell) != self.layout.get_room(
new_cell) or self.layout.has_passage(cell, new_cell)
if can_move:
self.place_player_at(new_cell)
Why? Because now all the layout references are in the top two lines. Now let’s extract those two two statements.
No, wait. If we do, we’ll have to return two results, new_cell and can_move.
I think what we need is something a bit different. We could do it from here but let’s reset and go again.
Refactoring Plan Two
The new plan is that whatever the method on layout is called, we’ll send it cell and offset and it will return either the new cell if it is valid, or the old cell if not. Our code in Dungeon will only move if the new cell is different from the old.
We begin here again:
def attempt_move_to_offset(self, cell, offset):
new_cell = self.layout.dungeon_cell_at_offset(cell.xy, offset)
if new_cell and self.can_move_between(cell, new_cell):
self.place_player_at(new_cell)
Start same way by extracting the ccan_move:
def attempt_move_to_offset(self, cell, offset):
new_cell = self.layout.dungeon_cell_at_offset(cell.xy, offset)
can_move = self.can_move_between(cell, new_cell)
if new_cell and can_move:
self.place_player_at(new_cell)
Now this goes by hand:
def attempt_move_to_offset(self, cell, offset):
new_cell = self.layout.dungeon_cell_at_offset(cell.xy, offset)
can_move = self.can_move_between(cell, new_cell)
if not can_move:
new_cell = cell
if new_cell != cell:
self.place_player_at(new_cell)
Why? Because now the last two lines are correct going forward. Our new method on layout will return the new cell if it can be moved to and otherwise the input cell.
Now inline can_move_between
def attempt_move_to_offset(self, cell, offset):
new_cell = self.layout.dungeon_cell_at_offset(cell.xy, offset)
can_move = not self.in_different_rooms(cell, new_cell) or self.layout.has_passage(cell, new_cell)
if not can_move:
new_cell = cell
if new_cell != cell:
self.place_player_at(new_cell)
Why? Because we want all the layout references up above those two correct lines. Inline again:
def attempt_move_to_offset(self, cell, offset):
new_cell = self.layout.dungeon_cell_at_offset(cell.xy, offset)
can_move = not self.layout.get_room(cell) != self.layout.get_room(new_cell) or self.layout.has_passage(cell, new_cell)
if not can_move:
new_cell = cell
if new_cell != cell:
self.place_player_at(new_cell)
You can almost certainly see where this is going. So can I. I’m trying to do as much of it as possible with machine refactorings. My reasons include:
- They are more reliable than manual changes and thus less likely to break things;
- Practice using them improves my skills;
- It’s rather a fun puzzle, figuring out the moves to use machine refactorings as much as we can.
I’ll leave it to you to assign weights to those reasons. I’m having too much fun to work it out.
Extract that whole top bit. Make up a name, time is fleeting.
def attempt_move_to_offset(self, cell, offset):
new_cell = self.cell_we_can_move_to(cell, offset)
if new_cell != cell:
self.place_player_at(new_cell)
def cell_we_can_move_to(self, cell, offset):
new_cell = self.layout.dungeon_cell_at_offset(cell.xy, offset)
can_move = (not self.layout.get_room(cell) != self.layout.get_room(new_cell)
or self.layout.has_passage(cell,new_cell))
if not can_move:
new_cell = cell
return new_cell
Now then. All the layout stuff is in cell_we_can_move_to, whose horrible name we’ll deal with soon.
I”d like to clean up that kind of mumbling finish in cell_we_can_move_to. I think we’ll wait until after the move. PyCharm is no help here, so I’ll change the attempt:
def attempt_move_to_offset(self, cell, offset):
new_cell = self.layout.cell_we_can_move_to(cell, offset)
if new_cell != cell:
self.place_player_at(new_cell)
Tests fail, of course. Grab the other method and convert it to work in layout:
class DungeonLayout:
def cell_we_can_move_to(self, cell, offset):
new_cell = self.dungeon_cell_at_offset(cell.xy, offset)
can_move = (not self.get_room(cell) != self.get_room(new_cell)
or self.has_passage(cell,new_cell))
if not can_move:
new_cell = cell
return new_cell
I just deleted all the .layout occurrences. Tests are green. Commit: removing feature envy.
Now let’s see about cleaning that up a bit.
Inline can_move?
def cell_we_can_move_to(self, cell, offset):
new_cell = self.dungeon_cell_at_offset(cell.xy, offset)
if not (not self.get_room(cell) != self.get_room(new_cell)
or self.has_passage(cell, new_cell)):
new_cell = cell
return new_cell
Just return cell instead of saving it:
def cell_we_can_move_to(self, cell, offset):
new_cell = self.dungeon_cell_at_offset(cell.xy, offset)
if not (not self.get_room(cell) != self.get_room(new_cell)
or self.has_passage(cell, new_cell)):
return cell
return new_cell
Add an else clause to help PyCharm help me:
def cell_we_can_move_to(self, cell, offset):
new_cell = self.dungeon_cell_at_offset(cell.xy, offset)
if not (not self.get_room(cell) != self.get_room(new_cell)
or self.has_passage(cell, new_cell)):
return cell
else:
return new_cell
No PyCharm will invert the if.
def cell_we_can_move_to(self, cell, offset):
new_cell = self.dungeon_cell_at_offset(cell.xy, offset)
if (not self.get_room(cell) != self.get_room(new_cell)
or self.has_passage(cell, new_cell)):
return new_cell
else:
return cell
Will it invert the other not? It will:
def cell_we_can_move_to(self, cell, offset):
new_cell = self.dungeon_cell_at_offset(cell.xy, offset)
if (self.get_room(cell) == self.get_room(new_cell)
or self.has_passage(cell, new_cell)):
return new_cell
else:
return cell
Tests are green. We can commit this: tidying.
But I have a concern! What if we were trying to move off-world? What will be in new_cell and what will this code do?
def dungeon_cell_at_offset(self, start_xy, offset_xy):
cell = self.at_offset(start_xy, offset_xy)
if not cell or self.is_available(cell):
return None
return cell
I introduced a defect at the very beginning, where I changed the if new_cell and to if new_cell != cell!
Shoulda coulda woulda, the question is what we should do now? The fix is easy:
def cell_we_can_move_to(self, cell, offset):
new_cell = self.dungeon_cell_at_offset(cell.xy, offset)
if new_cell and (self.get_room(cell) == self.get_room(new_cell)
or self.has_passage(cell, new_cell)):
return new_cell
else:
return cell
I’m confident enough to commit that. Fixed defect introduced in alleged refactoring.
But we need a test for the case:
def test_cell_we_can_move_to_on_border(self):
layout = DungeonLayout(5, 5)
cell = Cell(0, 0)
room = Room([cell], 'room')
move = layout.cell_we_can_move_to(cell,(-1,-1))
assert move is cell
Passes, now. Remove the check for a moment. It fails. Commit: add test for border case.
Nearly time to stop but I really don’t like that method. It is hard to read.
I’ll try something:
def cell_we_can_move_to(self, cell, offset):
new_cell = self.dungeon_cell_at_offset(cell.xy, offset)
if new_cell is None:
return cell
elif self.get_room(cell) == self.get_room(new_cell):
return new_cell
elif self.has_passage(cell, new_cell):
return new_cell
else:
return cell
I think that’s more clear than the and/or stuff. Commit: refactoring
I see things not to like. One is that this is the only use of dungeon_cell_at_offset.
A bit more work and:
def cell_we_can_move_to(self, cell, offset):
new_cell = self.at_offset(cell.xy, offset)
if self.are_in_same_room(cell, new_cell):
return new_cell
elif self.has_passage(cell, new_cell):
return new_cell
else:
return cell
def are_in_same_room(self, cell, new_cell):
return cell and new_cell and self.get_room(cell) == self.get_room(new_cell)
Now that is something we can be proud of, I think. Commit, then let’s sum up.
Summary
I made a serious error early on, replacing if new_cell and with if new_cell != cell, which would allow an off-world cell to drop through and ultimately move to the wrong place. Had I not noticed it and fixed it, I think we would have had a somewhat serious crash in the game. There was no test for the off-world case, so after fixing the defect that I introduced and committed, I wrote the test and verified that it found the problem.
I have no excuse and no explanation. I missed it.
It does pretty much underline one of my reasons for using machine refactorings: they are more reliable than I am.
Had I not made that mistake, this sequence would have been quite lovely, step after step, everything working, and a bunch of code from Dungeon migrating over to DungeonLayout, and finally turning into the eleven lines we see above.
Even so, the result is quite nice and aside from a two minute bit of embarrassment it all went smoothly and without stress.
Reflection
There are a couple of issues that come up for me, looking back over the morning:
-
There are a lot of methods in DungeonLayout about cell is this cell is that. This is feature envy: Cells are not very helpful. They have essentially no behavior and do not know anything beyond their coordinates. I do not see a way to improve that much, unless we gave them all a pointer back to the layout. And that seems wrong to me.
-
Similarly, if a Cell knew its Room that would sometimes be helpful. Again, we do not like to have objects pointing to their containers.
-
A low level design property of the current code is that methods returning a Cell can also return None, which means that a lot of other code then has to check that result. It is possible that a Missing Object approach might be helpful here. That might be an educational thing to work on.
This issue has kind of been a low-level ache right along. I would like to improve things. Ideas include:
- Make the cells into a four-way linked structure where every cell knows up to four neighbors.
- Perhaps there could be two kinds of links, direct and passage, with logic in them.
- Perhaps only cells that are in rooms actually exist. Cell available = cell not in any room
- Content is a dictionary in Dungeon from cell to a contents list. Maybe let the cell have its contents?
If this were a real product, there would be deadlines and product owners slavering at the door demanding more more more faster faster whip the ponies harder, and we would have little time to explore these larger issues.
But even then we could probably eke out a couple of hours from time to time to try things, and, if they seemed productive, to begin to move the product design in a better direction. If we can do that adeptly, our overall rate of production of good things will stay higher throughout the development effort, instead of slowing down more and more as the weight of old decisions bears down upon us, heavier and heavier … you get the idea.
But here, we’re more interested in the process we apply when time permits on a real project, where we take just a few hours and make things better. So … maybe we’ll look at this issue with Cell.
Or maybe we’ll do something else entirely. I can’t wait to find out. See you next time!