Hello, loves!

Writing a note to ask for ideas, I got an idea. Let’s see what happens with it. Result: I think it turns out nicely.

I was thinking about this morning’s creation of a little object that made things simpler and was easy to test. And I was looking at RoomView, and a long story test for something that doesn’t seem that tricky. And on Hill’s slack, I though I’d start a convo and get some advice, and I wrote:

A RoomView has a method illuminate that illuminates every cell that it contains. The RoomView is owned by the DungeonView, which has a list of illuminated cells, because when new content is added in game play, the content item needs to be illuminated or not depending on whether the cell it is being placed in is illuminated. The DungeonView knows the dungeon layout, since it runs the map and such. So a RoomView has a DungeonView … ah, there’s yer problem right there. Maybe we should just pass in the things the RoomView needs when it needs them, and then we could more readily test it by passing it fake things to talk to.

So let’s try to unhook the RoomView from knowing its owners, and in so doing, improve things a bit. Here’s the whole class:

class RoomView:
    def __init__(self, dungeon_view, room):
        self.dungeon_view = dungeon_view
        self.dungeon = dungeon_view.dungeon
        self.layout = self.dungeon.layout
        self.room = room
        self.sprites = []
        self.cell_sprites = dict()
        self.texture_finder = TextureFinder()

    def create_floor(self, shape_list):
        for cell in self.room:
            self.choose_flooring(cell, shape_list)

    def choose_flooring(self, cell, shape_list):
        texture = self.choose_flooring_texture(cell)
        sprite = self.make_adjusted_sprite(cell, texture)
        self.cell_sprites[cell] = sprite
        self.sprites.append(sprite)
        shape_list.append(sprite)

    def illuminate(self, center_cell, distance):
        for cell, sprite in self.cell_sprites.items():
            if cell.manhattan_distance(center_cell) < distance:
                self.dungeon_view.illuminate(cell)
                sprite.visible = True

    def make_adjusted_sprite(self, cell, texture):
        scale = scale_texture(texture)
        sprite = arcade.Sprite(texture, scale=scale)
        sprite.position = cell.center_position(cell_size)
        sprite.visible = False
        return sprite

    def choose_flooring_texture(self, cell):
        borders: BorderList = self.layout.get_borders(cell)
        border_type = borders.border_string()
        name = self.texture_finder.full_name(border_type)
        return arcade.load_texture(name)

The only user of layout is the last method (and a test). The only use of Dungeon is to get the layout. There is another use of the DungeonView inn illuminate. Let’s get rid of all three of those members.

First change signature of create_floor to require the layout as a parameter.

    def create_floor(self, layout, shape_list):
        for cell in self.room:
            self.choose_flooring(cell, shape_list)

I defaulted the new parameter to self.layout, and since the caller is DungeonView, it works. Tests are green. We could commit. Let’s do, just because we can. Commit: disconnecting RoomView from the universe.

Now change signature of choose_flooring:

    def choose_flooring(self, layout, cell, shape_list):
        texture = self.choose_flooring_texture(cell)
        sprite = self.make_adjusted_sprite(cell, texture)
        self.cell_sprites[cell] = sprite
        self.sprites.append(sprite)
        shape_list.append(sprite)

Green. Commit. Now change signature of choose_flooring_texture and remove the reference to self.layout.

Green. Commit. Now remove two member variables, dungeon and layout:

class RoomView:
    def __init__(self, dungeon_view, room):
        self.dungeon_view = dungeon_view
        self.room = room
        self.sprites = []
        self.cell_sprites = dict()
        self.texture_finder = TextureFinder()

I expect this to break a test and it does. The fix is to change this line in the test:

        borders = view.layout.get_borders(c_33)

To this:

        borders = layout.get_borders(c_33)

Green. Commit. Now for illuminate:

class RoomView:
    def illuminate(self, center_cell, distance):
        for cell, sprite in self.cell_sprites.items():
            if cell.manhattan_distance(center_cell) < distance:
                self.dungeon_view.illuminate(cell)
                sprite.visible = True

This wants a dungeon_view parameter. Change signature and remove the self:

    def illuminate(self, dungeon_view, center_cell, distance):
        for cell, sprite in self.cell_sprites.items():
            if cell.manhattan_distance(center_cell) < distance:
                dungeon_view.illuminate(cell)
                sprite.visible = True

A test fails. I am not surprised but hadn’t looked to see if one would. This line:

    def test_room_view_illuminate(self):
        ,,,
        room_view.illuminate(self, Cell(12, 12), 3)

Needs to be:

    def test_room_view_illuminate(self):
        ,,,
        room_view.illuminate(dungeon_view, Cell(12, 12), 3)

Green. Commit.

We can remove the saving of the dungeon_view instance variable:

class RoomView:
    def __init__(self, dungeon_view, room):
        self.room = room
        self.sprites = []
        self.cell_sprites = dict()
        self.texture_finder = TextureFinder()

Green. Commit. Change signature on the init:

class RoomView:
    def __init__(self, room):
        self.room = room
        self.sprites = []
        self.cell_sprites = dict()
        self.texture_finder = TextureFinder()

Green. Commit.

Reflection

We have removed three instance variables from RoomView, which were pointers that wired the class into everything around it. Now it is not wired to any other object, and when it needs information from or to send messages to another object, that object is passed to it on the relevant call. It is notably simpler.

PyCharm highlights this method as static, which is generally a hint of Feature Envy:

    def make_adjusted_sprite(self, cell, texture):
        scale = scale_texture(texture)
        sprite = arcade.Sprite(texture, scale=scale)
        sprite.position = cell.center_position(cell_size)
        sprite.visible = False
        return sprite

I don’t see a place we’d rather have the method. scale_texture is a method in ‘params.py’. We could move this method there, which would at least get it out of here. Let’s do that. Move it and edit in params:

params.py
def make_adjusted_sprite(cell, texture):
    scale = scale_texture(texture)
    sprite = arcade.Sprite(texture, scale=scale)
    sprite.position = cell.center_position(cell_size)
    sprite.visible = False
    return sprite

Change the call:

    def choose_flooring(self, layout, cell, shape_list):
        texture = self.choose_flooring_texture(layout, cell)
        sprite = params.make_adjusted_sprite(cell, texture)
        self.cell_sprites[cell] = sprite
        self.sprites.append(sprite)
        shape_list.append(sprite)

Green. Commit: move make_adjusted_sprite to params.

Reflection

RoomView is now vastly simplified:

class RoomView:
    def __init__(self, room):
        self.room = room
        self.sprites = []
        self.cell_sprites = dict()
        self.texture_finder = TextureFinder()

    def create_floor(self, layout, shape_list):
        for cell in self.room:
            self.choose_flooring(layout, cell, shape_list)

    def choose_flooring(self, layout, cell, shape_list):
        texture = self.choose_flooring_texture(layout, cell)
        sprite = params.make_adjusted_sprite(cell, texture)
        self.cell_sprites[cell] = sprite
        self.sprites.append(sprite)
        shape_list.append(sprite)

    def illuminate(self, dungeon_view, center_cell, distance):
        for cell, sprite in self.cell_sprites.items():
            if cell.manhattan_distance(center_cell) < distance:
                dungeon_view.illuminate(cell)
                sprite.visible = True

    def choose_flooring_texture(self, layout, cell):
        borders: BorderList = layout.get_borders(cell)
        border_type = borders.border_string()
        name = self.texture_finder.full_name(border_type)
        return arcade.load_texture(name)

Thirty lines, down from 40. No connection to substrate classes. We may or may not simplify its tests, but we certainly could if we chose to.

It’s not perfect, and it’s easier to see some flaws. Most significant issues include:

  1. It’s arguably not a view: it neither draws nor receives input from the model.
  2. It clearly has two entirely separate responsibilities: it creates flooring for the room’s cells, and it illuminates cells within the room.

This makes me suspect that we have, at best, some kind of conglomerate RoomUtilityFunctionCollection, and quite possibly two separate tiny classes, a RoomIlluminator and a FlooringSelector, or something like that.

But for now, we have made notable improvements, so let’s sum up.

Summary

Here, we observed a class that had accumulated links to the DungeonView, the Dungeon (which it really didn’t need) and to the DungeonLayout. No one deserves that much explicit connectivity. Removing the object’s links and passing in reference objects only as needed reduces coupling substantially, at no cost in additional code, merely changes to calling sequences and subsequent references to parameters instead of instance variables.

It may not be worth it to go back and retrofit the tests, but I’d wager that we can write much simpler tests for RoomView now than we currently have.

I wonder … the Cell now knows how to access the layout, which it does through a Cell class variable. Right now, we don’t make use of that link, but we could. Probably not a good idea: still better to pass it where it’s needed.

Or maybe there are new methods that could be added to Cell that would be helpful. We’ll keep an eye out for that, but we want not to overload Cell with things that don’t really pertain to it. But worth keeping in mind.

Bottom line: a noticeable improvement and very easy to accomplish. Something to think about.

See you next time!