Refactoring II
We continue the prior article’s session in a new article but same session, refactoring another bit of code that could use a little improvement.
This is a follow-on article to Refactoring I, which got too long and I’ve split it into two articles of which this, in case you haven’t worked it out yet, is the second of two.
Let’s try one more refactoring this morning. How about our new code for finding continents? As of this morning, that looks like this:
def find_continents(self):
continents = []
rooms = self.rooms.copy()
while rooms:
start_room = rooms.pop()
continent = self.find_connected_rooms(start_room)
continents.append(continent)
for room in [room for room in continent if room in rooms]:
rooms.remove(room)
return continents
def find_connected_rooms(self, room):
connected_rooms = {room}
frontier = [room.cells[0]]
reached = set()
while frontier:
cell = frontier.pop()
neighbor_cells = [cell for cell in cell.neighbors
if cell.is_in_a_room()]
for neighbor_cell in neighbor_cells:
connected_rooms.add(neighbor_cell.owner)
if neighbor_cell not in reached:
reached.add(neighbor_cell)
frontier.append(neighbor_cell)
return connected_rooms
Let’s begin with a little renaming, to the suites notion:
def define_suites(self):
suites = []
rooms = self.rooms.copy()
while rooms:
start_room = rooms.pop()
suite = self.explore_suite(start_room)
suites.append(suite)
for room in [room for room in suite if room in rooms]:
rooms.remove(room)
return suites
def explore_suite(self, room):
suite = {room}
frontier = [room.cells[0]]
reached = set()
while frontier:
cell = frontier.pop()
neighbor_cells = [cell for cell in cell.neighbors
if cell.is_in_a_room()]
for neighbor_cell in neighbor_cells:
suite.add(neighbor_cell.owner)
if neighbor_cell not in reached:
reached.add(neighbor_cell)
frontier.append(neighbor_cell)
return suite
Just some renaming. Commit: renaming.
I wonder if we can get rid of the if room in rooms in define_suites. Yes. The check is finding our starting room, which we remove with the pop. So this passes:
def define_suites(self):
suites = []
rooms = self.rooms.copy()
while rooms:
start_room = rooms[0] # <===
suite = self.explore_suite(start_room)
suites.append(suite)
for room in [room for room in suite]: # <===
rooms.remove(room)
return suites
Remove is probably slow as Python will look for each member.
def define_suites(self):
suites = []
rooms = self.rooms.copy()
while rooms:
start_room = rooms[0]
suite = self.explore_suite(start_room)
suites.append(suite)
rooms = [room for room in rooms if room not in suite]
return suites
Still green. Commit, tidying.
Extract the inside of that while loop.
def define_suites(self):
suites = []
rooms = self.rooms.copy()
while rooms:
rooms = self.create_suites(rooms, suites)
return suites
def create_suites(self, rooms: list[Any], suites: list[Any]) -> list[Any]:
start_room = rooms[0]
suite = self.explore_suite(start_room)
suites.append(suite)
rooms = [room for room in rooms if room not in suite]
return rooms
A bit of renaming:
def create_suites(self, rooms: list[Any], suites: list[Any]) -> list[Any]:
first_room = rooms[0]
suite = self.find_connected_rooms(first_room)
suites.append(suite)
rooms = [room for room in rooms if room not in suite]
return rooms
I think I went back to my original name, find_connected_rooms there. Interesting. I’ll allow it.
Commit. Inline a bit. Also a rename of the method to singular:
def define_suites(self):
suites = []
rooms = self.rooms.copy()
while rooms:
rooms = self.create_suite(rooms, suites)
return suites
def create_suite(self, rooms: list[Any], suites: list[Any]) -> list[Any]:
suite = self.find_connected_rooms(rooms[0])
suites.append(suite)
return [room for room in rooms if room not in suite]
It just creates one suite at a time, not suites. And create isn’t the right name. It adds a new suite.
def define_suites(self):
suites = []
rooms = self.rooms.copy()
while rooms:
rooms = self.add_suite(rooms, suites)
return suites
def add_suite(self, rooms: list[Any], suites: list[Any]) -> list[Any]:
suite = self.find_connected_rooms(rooms[0])
suites.append(suite)
return [room for room in rooms if room not in suite]
Better, I think. See what you think and let me know if you wish. Now to that find method:
def find_connected_rooms(self, room):
suite = {room}
frontier = [room.cells[0]]
reached = set()
while frontier:
cell = frontier.pop()
neighbor_cells = [cell for cell in cell.neighbors
if cell.is_in_a_room()]
for neighbor_cell in neighbor_cells:
suite.add(neighbor_cell.owner)
if neighbor_cell not in reached:
reached.add(neighbor_cell)
frontier.append(neighbor_cell)
return suite
It’s perhaps not obvious that this is a flood fill that just checks for rooms. We’ll see if we can make that more clear as we go.
I notice this:
neighbor_cells = [cell for cell in cell.neighbors
if cell.is_in_a_room()]
Demeter feels violated. Let’s ask the cell for neighbors in rooms:
def find_connected_rooms(self, room):
suite = {room}
frontier = [room.cells[0]]
reached = set()
while frontier:
cell = frontier.pop()
# change below
neighbor_cells = cell.neighbors_in_rooms()
for neighbor_cell in neighbor_cells:
suite.add(neighbor_cell.owner)
if neighbor_cell not in reached:
reached.add(neighbor_cell)
frontier.append(neighbor_cell)
return suite
class Cell:
def neighbors_in_rooms(self):
return [neighbor
for neighbor in self.neighbors
if neighbor.is_in_a_room()]
Commit.
Inline:
def find_connected_rooms(self, room):
suite = {room}
frontier = [room.cells[0]]
reached = set()
while frontier:
cell = frontier.pop()
for neighbor_cell in cell.neighbors_in_rooms():
suite.add(neighbor_cell.owner)
if neighbor_cell not in reached:
reached.add(neighbor_cell)
frontier.append(neighbor_cell)
return suite
Commit.
Is cell.owner obviously the room? I think not. Let’s have a room method on cell.
class Dungeon:
...
suite.add(neighbor_cell.room)
...
class Cell:
def room(self):
return self.owner
Commit. Looking at the whole method:
def find_connected_rooms(self, room):
suite = {room}
frontier = [room.cells[0]]
reached = set()
while frontier:
cell = frontier.pop()
for neighbor_cell in cell.neighbors_in_rooms():
suite.add(neighbor_cell.room())
if neighbor_cell not in reached:
reached.add(neighbor_cell)
frontier.append(neighbor_cell)
return suite
We note, among other things, that the method does not refer to self at all. PyCharm suggests that it could be static or a stand-alone function, and it’s not wrong. What we have here is quite possibly calling out for the Method Object pattern, and/or a new class to handle floods for us.
That’s a larger step than I want to undertake right now. I wonder whether we could move the suite.add under the if. I think we can. I’m not sure I trust my single test quite that much, though.
It does pass. We’ll commit with a note that we want more tests for this capability.
def find_connected_rooms(self, room):
suite = {room}
frontier = [room.cells[0]]
reached = set()
while frontier:
cell = frontier.pop()
for neighbor_cell in cell.neighbors_in_rooms():
if neighbor_cell not in reached:
suite.add(neighbor_cell.room())
reached.add(neighbor_cell)
frontier.append(neighbor_cell)
return suite
Reflection.
I started about 1000 and it is just gone 1200 now, so let’s call a break. Here’s the code as it stands: you can compare it with the original up above:
def define_suites(self):
suites = []
rooms = self.rooms.copy()
while rooms:
rooms = self.add_suite(rooms, suites)
return suites
def add_suite(self, rooms: list[Any], suites: list[Any]) -> list[Any]:
suite = self.find_connected_rooms(rooms[0])
suites.append(suite)
return [room for room in rooms if room not in suite]
def find_connected_rooms(self, room):
suite = {room}
frontier = [room.cells[0]]
reached = set()
while frontier:
cell = frontier.pop()
for neighbor_cell in cell.neighbors_in_rooms():
if neighbor_cell not in reached:
suite.add(neighbor_cell.room())
reached.add(neighbor_cell)
frontier.append(neighbor_cell)
return suite
There are oddities here. The while on rooms and the return of rooms from add_suite strikes me as not entirely obvious. The issue is that the code changes two lists, the suites and the rooms. If we were to change rooms in place, it might be better, or it might be even more obscure. Maybe we’ll try it later.
I think add_suite is pretty good, although it has the odd return of rooms that makes it kind of have two functions instead of just one. Its proper name would be something like “add suite and update rooms to remove rooms in the suite”.
The final method is entirely without reference to self and I think it calls for method object or the equivalent. We’ll undertake that next time, which will be tomorrow.
So … if you care to … reflect on whether the changes made here, and in the previous article, make sense to you. There is no right and wrong here: these are the choices I made and I like the result better than what we had, but I am not in love with any of them. If I were in love with them, they’d probably be too clever to be allowed to live. But I like them better.
And tomorrow, we’ll try for something that I like even better.
Feel free to toot me on Mastodon if you are so inclined. See you next time!