Refactoring III
What we need here, I reckon, is Method Object. Let’s try it. Oddly enough, that’s actually what happens. There might be something better still …
When last we looked at the code, this morning, we had this code:
class Dungeon:
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
That last method, find_connected_rooms, does not refer to self, the dungeon, at all. That tells us that it doesn’t really belong as a method on Dungeon. Where does it belong instead? You could make a case that it belongs on Cell, since it does call a couple of methods on Cell, neighbors_in_rooms and room. But what the method is about seems to be managing the frontier, the reached set and possibly suite.
I think it belongs in a class of its own. When we discover a situation like this, the Method Object pattern invites us to create a new class that deals with whatever matters the method seems to be concerned with. Let’s try that.
I have a feeling that the adding of the room to the suite should be out of scope for this object, which would otherwise just present cells to be processed. What if we passed in a function to be called, from add_suite? Let’s leave that aside, it seems tricky. We’ll create our object with all the variables in it, then move the method over:
class Dungeon:
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):
return SuiteFinder(room).find_suite()
class SuiteFinder:
def __init__(self, room):
self.room = room
self.suite = {room}
self.frontier = [room.cells[0]]
self.reached = set()
def find_suite(self):
while self.frontier:
cell = self.frontier.pop(0)
for neighbor_cell in cell.neighbors_in_rooms():
if neighbor_cell not in self.reached:
self.suite.add(neighbor_cell.room())
self.reached.add(neighbor_cell)
self.frontier.append(neighbor_cell)
return self.suite
PyCharm seemed to have a good idea what I was trying to do, and prompted with suggested lines that were often close to what I needed, and sometimes actually what I needed. I’ve found that even without the “AI” turned on, I have to be careful. PyCharm is often right and sometimes subtly wrong.
Let’s inline some code in Dungeon:
class Dungeon:
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 = SuiteFinder(rooms[0]).find_suite()
suites.append(suite)
return [room for room in rooms if room not in suite]
In fact, let’s inline further.
def define_suites(self):
suites = []
rooms = self.rooms.copy()
while rooms:
suite = SuiteFinder(rooms[0]).find_suite()
suites.append(suite)
rooms = [room for room in rooms if room not in suite]
return suites
I almost like that. The final working line is really removing all the rooms found in the current suite from the working copy. Let’s first try a rename:
def define_suites(self):
suites = []
unexplored = self.rooms.copy()
while unexplored:
suite = SuiteFinder(unexplored[0]).find_suite()
suites.append(suite)
unexplored = [room for room in unexplored if room not in suite]
return suites
And I really think that suites wants to be a member.
def define_suites(self):
self.suites = []
unexplored = self.rooms.copy()
while unexplored:
suite = SuiteFinder(unexplored[0]).find_suite()
self.suites.append(suite)
unexplored = [room for room in unexplored if room not in suite]
return self.suites
And I noticed we don’t use room in the SuiteFinder, so:
class SuiteFinder:
def __init__(self, room):
self.suite = {room}
self.frontier = [room.cells[0]]
self.reached = set()
def find_suite(self):
while self.frontier:
cell = self.frontier.pop(0)
for neighbor_cell in cell.neighbors_in_rooms():
if neighbor_cell not in self.reached:
self.suite.add(neighbor_cell.room())
self.reached.add(neighbor_cell)
self.frontier.append(neighbor_cell)
return self.suite
We had about 25 lines, and we have, I think, 23 now. But more to the point, We have a little SuiteFinder class that helps out The Dungeon, and I think both the Dungeon code and the SuiteFinder are pretty clear.
I think we could actually move all the variables inside the method, couldn’t we? That would be equivalent to making it a free function, except that it would be embedded in a class.
We might try making a free function at some future time. For now I am pleased with the result, which was, for a change, pretty much what I had intended to do.
It could happen. See you next time!