There is Massive Duplication in my texture-finding code. I plan to remove that. It goes rather nicely.

Here’s the duplication that I have in mind:

class RoomView:
    path = '/Users/ron/PycharmProjects/dungeon/assets/images/'
    texture_dictionary = {
        '': arcade.load_texture(path+'Tile (21).png'), # none
        'E': arcade.load_texture(path+'Floor_Edge_29.png'), # east
        'N': arcade.load_texture(path+'Floor_Edge_36.png'), # north
        'EN': arcade.load_texture(path+'Wall_81.png'),
        'W': arcade.load_texture(path+'Floor_Edge_33.png'), # west
        'EW': arcade.load_texture(path+'Wall_67.png'),
        'NW': arcade.load_texture(path+'Wall_80.png'),
        'ENW': arcade.load_texture(path+'Wall_66.png'),
        'S': arcade.load_texture(path+'Floor_Edge_25.png'), # south
        'ES': arcade.load_texture(path+'Wall_55.png'),
        'NS': arcade.load_texture(path+'Wall_3.png'),
        'ENS': arcade.load_texture(path+'Wall_4.png'),
        'WS': arcade.load_texture(path+'Wall_53.png'),
        'EWS': arcade.load_texture(path+'Wall_69.png'),
        'NWS': arcade.load_texture(path+'Wall_1.png'),
        'ENWS': arcade.load_texture(path+'Wall_57.png'),
    }

As all here have probably heard me say before, duplication in code is a cry for improvement. It almost invariably suggests that there is at least a “missing” method that would be helpful, and often a “missing” small class. But, someone might say, it’s all right there and isn’t hurting anyone, why should we change it?

Well, to begin with, the code there obscures the important thing, which is the mapping between a border style, say ‘ES’, and its corresponding tile texture, ‘Wall-55.png’. We would like our code to be easy to understand, and the closer we can get to saying 'ES':'Wall_55.png' the easier it will be to understand it.

My experience is that as we make changes to remove duplication, we almost always wind up with code that is easier to work with, and more likely to be useful in more than one situation. And, for me, there is great value in producing code that doesn’t cause people to scream “WHO THE HELL WROTE THIS LET ME AT HIM!”

No, but seriously, I do prefer to do good work, to the extent that I can.

So we’re gonna do it. Let’s think a bit more about what we see here. We actually have two bits of duplication, the arcade.load_texture, and the path+. The path is a separate class variable than the texture-dictionary, and yet clearly is part of what’s going on.

Furthermore, what we have here are class variables, compiled into the RoomView class. We are pretty much stuck with that path and those file names. We don’t like to rely much on speculation, but because we already have water tiles, mud tiles and the stone-looking tiles we’re using mostly now, it’s not very risky to anticipate that we’ll want to provide different rooms with different tile sets.

This much thinking, which required less time to do than to write it up suggests to me that we might want to move toward a separate class for finding textures. We wouldn’t necessarily do it now, but we wouldn’t necessarily not do it now. But we do have time to remove some duplication, so we’ll do that.

Springing into action, we see that there is exactly one reference to the texture dictionary, in RoomView:

    def choose_flooring_texture(self, cell):
        borders: BorderList = self.layout.get_borders(cell)
        border_type = borders.border_string()
        if border_type == '':
            return random.choices(self.textures, self.weights)[0]
        else:
            return self.texture_dictionary[border_type]

What do we want? I claim that what we want is to remove the duplicated arcade.load_texture and path+ from that dictionary. And we’d like to do it in very small steps. And we’d like to do it with very few mistakes, ideally zero.

In support of that, let’s write a trivial test, so that we don’t have to run the program to check that we’re OK.

Arrgh!

Well, that didn’t work out. Look at what I had to do just to get the texture, and by then it is some arcane arcade thing that I have no useful way of testing:

    def test_choose_flooring_texture(self):
        layout = DungeonLayout(10, 10)
        cell = Cell(1, 1)
        room = Room([cell])
        layout.add_room(room)
        dungeon = Dungeon(layout)
        room_view = RoomView(dungeon, room)
        texture = room_view.choose_flooring_texture(cell)
        # assert something about texture

creating a layout, a cell, a room, a dungeon and a view just to check what amounts to a string? This won’t do. I’ll write a simpler test and make it work.

    def test_choose_flooring_texture(self):
        room_view = RoomView(None, None)
        name = room_view.flooring_texture_name('ENWS')
        assert name == 'Wall_57.png'

We have two issues here. One is that we cannot create a RoomView with no dungeon, because it fetches a layout. The other, simpler one, is that we don’t have a method flooring_texture_name.

All this is telling me that I’m not working in the right space. Even this simpler test isn’t simple enough to get at what I want to test.

OK. We’ll TDD up a new class and plug it in. New test file.

class TextureFinder:
    pass


class TestTextureFinder:
    def test_exists(self):
        tf = TextureFinder()

Perfect. Green. Commit: building TextureFinder.

Now we’re rolling! Can you feel the freedom? Yes!

Presumably a TextureFinder needs a path and a dictionary like the one we have in RoomView, and I think for now we’ll just include them as a default in the class, mostly because I’m not ready to think about where these things come from.

Let’s just write a test for the short name of the file. This may be an internal method by the time we’re done.

class TestTextureFinder:
    def test_exists(self):
        tf = TextureFinder()
        name = tf.short_name('ENWS')
        assert name == 'Wall_57.png'

I decided to use Fake It Till You Make It here:

class TextureFinder:
    def short_name(self, borders):
        return 'Wall_57.png'

Green. Commit. Another test to drive out some actual work:

    def test_NWS(self):
        tf = TextureFinder()
        name = tf.short_name('NWS')
        assert name == 'Wall_1.png'

I’m taking these names from the current “real” dictionary, by the way. Now, I could just put an if statement in short_name but that seems wrong. Let’s init a bit. I’ll copy the whole texture dictionary over.

class TextureFinder:
    def __init__(self):
        self.names = {
        '': arcade.load_texture(path+'Tile (21).png'), # none
        'E': arcade.load_texture(path+'Floor_Edge_29.png'), # east
        'N': arcade.load_texture(path+'Floor_Edge_36.png'), # north
        'EN': arcade.load_texture(path+'Wall_81.png'),
        'W': arcade.load_texture(path+'Floor_Edge_33.png'), # west
        'EW': arcade.load_texture(path+'Wall_67.png'),
        'NW': arcade.load_texture(path+'Wall_80.png'),
        'ENW': arcade.load_texture(path+'Wall_66.png'),
        'S': arcade.load_texture(path+'Floor_Edge_25.png'), # south
        'ES': arcade.load_texture(path+'Wall_55.png'),
        'NS': arcade.load_texture(path+'Wall_3.png'),
        'ENS': arcade.load_texture(path+'Wall_4.png'),
        'WS': arcade.load_texture(path+'Wall_53.png'),
        'EWS': arcade.load_texture(path+'Wall_69.png'),
        'NWS': arcade.load_texture(path+'Wall_1.png'),
        'ENWS': arcade.load_texture(path+'Wall_57.png'),
    }

This does not please anyone, but a simple edit gives me this:

class TextureFinder:
    def __init__(self):
        self.names = {
        '': 'Tile (21).png', # none
        'E': 'Floor_Edge_29.png', # east
        'N': 'Floor_Edge_36.png', # north
        'EN': 'Wall_81.png',
        'W': 'Floor_Edge_33.png', # west
        'EW': 'Wall_67.png',
        'NW': 'Wall_80.png',
        'ENW': 'Wall_66.png',
        'S': 'Floor_Edge_25.png', # south
        'ES': 'Wall_55.png',
        'NS': 'Wall_3.png',
        'ENS': 'Wall_4.png',
        'WS': 'Wall_53.png',
        'EWS': 'Wall_69.png',
        'NWS': 'Wall_1.png',
        'ENWS': 'Wall_57.png',
    }

And then this:

    def short_name(self, borders):
        return self.names[borders]

Green. Commit. Let’s test for the full path name. Again I plan to use the actual value that is in RoomView.

    def test_full_name(self):
        tf = TextureFinder()
        full_name = tf.full_name('EWS')
        assert full_name == '/Users/ron/PycharmProjects/dungeon/assets/images/Wall_69.png'

Sorry about the long line there, but I’m trying to make minimal changes. We need a full_name method, and a path to append to.

class TextureFinder:
    def __init__(self):
        self.path = '/Users/ron/PycharmProjects/dungeon/assets/images/'
        self.names = {
        '': 'Tile (21).png', # none
        'E': 'Floor_Edge_29.png', # east
        'N': 'Floor_Edge_36.png', # north
        'EN': 'Wall_81.png',
        'W': 'Floor_Edge_33.png', # west
        'EW': 'Wall_67.png',
        'NW': 'Wall_80.png',
        'ENW': 'Wall_66.png',
        'S': 'Floor_Edge_25.png', # south
        'ES': 'Wall_55.png',
        'NS': 'Wall_3.png',
        'ENS': 'Wall_4.png',
        'WS': 'Wall_53.png',
        'EWS': 'Wall_69.png',
        'NWS': 'Wall_1.png',
        'ENWS': 'Wall_57.png',
    }

    def short_name(self, borders):
        return self.names[borders]

    def full_name(self, borders):
        return self.path + self.names[borders]

Green. Commit: TextureDictionary returns correct short and long resource strings.

So that was easy. And I think we can plug it into RoomView like this:

class RoomView:
    def __init__(self, dungeon, room):
        self.dungeon = dungeon
        self.layout = dungeon.layout
        self.room = room
        self.texture_finder = TextureFinder()
        ...

    def choose_flooring_texture(self, cell):
        borders: BorderList = self.layout.get_borders(cell)
        border_type = borders.border_string()
        if border_type == '':
            return random.choices(self.textures, self.weights)[0]
        else:
            name = self.texture_finder.full_name(border_type)
            return arcade.load_texture(name)

We can, and the game draws correctly. Commit: using TextureFinder in RoomView.

Reflection

We had a couple of quick false starts. I realized that writing a test for texture-finding by testing RoomView wasn’t really feasible, since the only method I had at that time was returning an Arcade Texture, which has nothing really testable as far as I know. Maybe it does but the whole test was So Ugly.

Even the simpler test I tried to write left me with the same issue, so it finally dawned on me that rather than start at the RoomView and refactor, we’d do better to gin up the little object we needed, in simple form, then plug it in.

And that just went as smoothly as one could wish, and plugging it in came down to about a three line change, plus a massive deletion, in RoomView. Very nice.

Further Reflection

There’s more to do. First of all, let me show you the rest of RoomView’s init:

class RoomView:
    def __init__(self, dungeon, room):
        self.dungeon = dungeon
        self.layout = dungeon.layout
        self.room = room
        self.texture_finder = TextureFinder()
        path = '/Users/ron/PycharmProjects/dungeon/assets/images/'
        f21 = arcade.load_texture(path + 'Tile (21).png')
        fgrate = arcade.load_texture(path + 'Floor_13_Grateb.png')
        f22 = arcade.load_texture(path + 'Tile (22).png')
        self.fnorth = arcade.load_texture(path + 'Tile (36).png')
        self.textures = [f21, f22, fgrate]
        self.weights = [20, 5, 1]

That additional rigmarole is there to provide random floor tiles, with the occasional grate or cracked tile. I think fnorth is no longer used. Right. Remove it. Commit.

As I was saying, the rigmarole sets up a list of tiles and a list of weights, which is used in the if-part of our choosing method:

    def choose_flooring_texture(self, cell):
        borders: BorderList = self.layout.get_borders(cell)
        border_type = borders.border_string()
        if border_type == '':
            return random.choices(self.textures, self.weights)[0]
        else:
            name = self.texture_finder.full_name(border_type)
            return arcade.load_texture(name)

Let’s improve that while we’re here. Since our choose method is handling the load in one branch we can do the same if we just choose among the path names of those tiles:

class RoomView:
    def __init__(self, dungeon, room):
        self.dungeon = dungeon
        self.layout = dungeon.layout
        self.room = room
        self.texture_finder = TextureFinder()
        path = '/Users/ron/PycharmProjects/dungeon/assets/images/'
        f21 = path + 'Tile (21).png'
        fgrate = path + 'Floor_13_Grateb.png'
        f22 = path + 'Tile (22).png'
        self.textures = [f21, f22, fgrate]
        self.weights = [20, 5, 1]

    def choose_flooring_texture(self, cell):
        borders: BorderList = self.layout.get_borders(cell)
        border_type = borders.border_string()
        if border_type == '':
            name = random.choices(self.textures, self.weights)[0]
            return arcade.load_texture(name)
        else:
            name = self.texture_finder.full_name(border_type)
            return arcade.load_texture(name)

That works. Commit. Remove duplication:

    def choose_flooring_texture(self, cell):
        borders: BorderList = self.layout.get_borders(cell)
        border_type = borders.border_string()
        if border_type == '':
            name = random.choices(self.textures, self.weights)[0]
        else:
            name = self.texture_finder.full_name(border_type)
        return arcade.load_texture(name)

Now that’s better … and it also points to some opportunities. It might be nice if …

  • Clearly we’ll want different TextureFinders, depending on room style, stone, muddy, water, etc.
  • We could put a list of textures with weights into the texture dictionary;
  • We could put other keys and names, such as f21 and Tile (21).png in there;
  • Oh … here’s an idea …

Let’s expose a method in TextureFinder to build the full path for us. Let’s test it into existence.

    def test_add_path(self):
        tf = TextureFinder()
        full_name = tf.add_path('xxx.png')
        assert full_name == '/Users/ron/PycharmProjects/dungeon/assets/images/xxx.png'

And the method, including internal use:

class TextureFinder:
    def short_name(self, borders):
        return self.names[borders]

    def full_name(self, borders):
        return self.add_path(self.names[borders])

    def add_path(self, short_name):
        return self.path + short_name

Now back in RoomView:

class RoomView:
    def __init__(self, dungeon, room):
        self.dungeon = dungeon
        self.layout = dungeon.layout
        self.room = room
        self.texture_finder = TextureFinder()
        f21 = 'Tile (21).png'
        fgrate = 'Floor_13_Grateb.png'
        f22 = 'Tile (22).png'
        self.textures = [f21, f22, fgrate]
        self.weights = [20, 5, 1]

    def choose_flooring_texture(self, cell):
        borders: BorderList = self.layout.get_borders(cell)
        border_type = borders.border_string()
        if border_type == '':
            short_name = random.choices(self.textures, self.weights)[0]
            name = self.texture_finder.add_path(short_name)
        else:
            name = self.texture_finder.full_name(border_type)
        return arcade.load_texture(name)

Works, commit. One more thing, inline those weird names:

class RoomView:
    def __init__(self, dungeon, room):
        self.dungeon = dungeon
        self.layout = dungeon.layout
        self.room = room
        self.texture_finder = TextureFinder()
        self.textures = ['Tile (21).png', 'Tile (22).png', 'Floor_13_Grateb.png']
        self.weights = [20, 5, 1]

Summary

We saw an opportunity to simplify the code and took it. Our choose method here, and the parts of the init that correspond to it, are telling us that a more capable texture finder could probably reduce our texture-finding work in RoomView down to a single call, eliminating our special check for no borders, and perhaps eliminating making that check anywhere.

One way of looking at this is that we are moving from procedural code handling things, toward data structures handling those things, with more of the logic of the situation represented in data, and less in code. That’s very often rather nice, and I think we’re seeing a bit of that here already.

We have plenty of options ahead of us, and there’s code that would like to be simpler. But we have moved in a good direction.

I’m reminded of that test. I’ll add a skipped dummy test, to nag me until I do the actual test.

    @pytest.mark.skip('write this test')
    def test_missing_key(self):
        assert False

A noticeable improvement, no stress, just simple tests and simple moves. I love when that happens.

See you next time!