GitHub Repo

This morning I plan to encapsulate my map of Item instances, and show why I think it’s a good idea. In addition, I’ll clarify what I mean by ‘always’ and ‘never’.

TL:DR
In this article, I replace a couple of Maps with instances of a new class, Items. The result is simpler code for the users of Items, supported by very simple code inside Items itself.

Enclosing the native collection makes the code more cohesive, simpler, and easier to maintain. That’s why I think it’s a good idea to cover native collections.

If you’re feeling impatient, skip to “My Take” down at the bottom. I think there’s value in seeing how easy it was, but you do you.

Always and Never

In an earlier article, I said:

And I’m going to try to make and keep a new resolution:

Never, ever, use a native collection directly, even “just for a moment to save time”. Always, always, create an object of your own, with a sensible name and sensible get (and put) methods of its own.

Now other than eat more chips than I ought to, it’s fair to say that I “never” do anything “always”, nor do I “ever” do anything “never”. There are always(!) exceptions to any rule that I follow, although so far I’ve never broken the one about picking up scorpions. As for programming “rules”, some are good, some less so, but few of them should be followed all the time. First of all, I believe that we should always(!) be thinking about what we’re doing, deciding what to do next and how to do it. While habits and reactions are good, conscious consideration is, in my view, better.

That said, of all the rules guidelines I’m aware of, the one above, about encapsulating native collections, seems to me to be a particularly good one.

And yet, in recent Twitter chatter, some folks are wondering why. So, this morning, since we happen to have a few native collections in my ADVENTure program, we’ll wrap them and see what happens. If you’re wondering whether and when to wrap a collection, maybe this will give you something to consider.

Things in the World

Our little game has what we’re calling “Item”. An Item has a short key name, like “axe”, and that key is unique: there’s only supposed to be one thing named “axe”. The Item also has a few other elements, a short and long description, and I expect that it will have more.

Originally, the Item was just a String: “axe”, kept in a Kotlin MutableSet. I needed more capability than just the String, so I created the Item class and have begun putting stuff into it.

There are two places where an Item might be. In might be in a Room, or the player might have it in her “inventory”. The way I’ve implemented that is that each Room has a collection, contents and the World holds on to the inventory, in a collection called inventory.

Originally, each these collections was a MutableSet of String. That was enough to keep the items unique and it was easy to find out whether there was an axe in a given room’s contents, and to move it to the inventory if the player took it.

When I changed over to Item, it made more sense to make those collections into MutableMaps, from String to Item. The Map enforces the uniqueness of the key, and the value is the whole Item.

That changeover required me to change those two variables from MutableSet to MutableMap, and to change the various methods that accessed contents or inventory. No big deal, just took me a little while. I think that was yesterday. As we’ll talk about later, it might have gone better another way, but it was fine.

Today, I plan to change those collections to an object of my own, which I intend to call Items signifying that it is a collection containing Items. (Some might want to call it ItemCollection. That would be OK with me.)

We’ll be looking at a few angles as we do this, mostly about how we do it, what changes when we do it, whether we like the old way or the new way better, and what might have happened had we done this sooner.

Let’s get started.

Where We Were, Where We Are

There are two occurrences of this MutableMap n the code, one in World, and one in Room. (The latter has many instances, of course.)

class World ...
    private val inventory: Items = mutableMapOf<String,Item>()


class Room(val roomName: String) {
    val contents = mutableMapOf<String, Item>()

Now we notice something right away. The World one refers to Items, the Room one does not. What is Items right now?

typealias Items = MutableMap<String, Item>

Just shorthand. But for some reason I didn’t refer to it in Room. I think it’d be better to do so, so:

class Room(val roomName: String) {
    val contents: Items = mutableMapOf<String, Item>()

OK, that documents things a bit better, makes it more clear that inventory and contents are the same kind of thing, the same idea. Tests are green, of course, so Commit: Room.contents declared as Items typealias.

When we look at references to these variables, they’re not unreasonable, but they are in more than one place:

class Room ...
    fun itemString(): String {
        return contents.values.joinToString(separator = "") {"You find ${it.shortDesc}.\n"}
    }

    val take = { imperative: Imperative, world: World ->
        val item = contents.remove(imperative.noun)
        if ( item!=null ) {
            world.addToInventory(item)
            world.response.say("${item.name} taken.")
        } else {
            world.response.say("I see no ${imperative.noun} here!")
        }
    }

    fun item(thing: String, details: Item.()->Unit): Item {
        val item = Item(thing)
        contents[thing] = item
        item.details()
        return item
    }

class RoomTest
        ...
        assertThat(room.contents).containsKey("broom")

class ItemTest
        val item: Item = myRoom.contents["axe"]!!

That’s just the references to contents. Now for inventory:

class World ...
    fun inventoryHas(item: String): Boolean {
        return inventory.contains(item)
    }

    private fun showInventory() {
        say( inventory.values.joinToString(prefix="You have ", transform={it.shortDesc}, separator=", ", postfix=".\n") )
    }

    fun addToInventory(item:Item) {
        inventory[item.name] = item
    }

Now, looking back, when I changed from the Set to the Map, every one of those methods, all over, needed to be looked at and changed. (I think one of them didn’t have to change, but I’m not sure.) It wasn’t hard: the program is small. If the program was larger, the task would grow with it. The chance that I’d get all the changes right would decline. Bugs might arise. Chaos might ensue. Dogs and cats might start living together. Mass hysteria. You know how it goes.

But we’re all good programmers here and we worked through it just fine.

Today, I’m going to wrap that Map in an object and we’ll see what happens.

I expect that our existing tests will be sufficient to drive out any problems with this change, especially since Kotlin and IDEA are all about checking to make sure I don’t set a foot wrong.

One more thing: I’m going to do this, as well as I can, to minimize errors. As such, I’m going to first build the Items object so that it emulates what a Map can do, and then slowly bring things inside.

I think there’s a clever way to do that with Kotlin’s by keyword. I’ll try that briefly but if I can’t figure it out, I’ll do it the hard way, which isn’t much harder.

Foreshadowing
It doesn’t work. In a different style of article, you’d never know that I even tried the idea. That’s not how we do things here: you get to see my mistakes.

What follows is a bit experimental. I create a simple class Items, to replace my typealias:

class Items(map: ItemMap = mutableMapOf<String, Item>()) {
}

That requires me to slightly rephrase the declarations of contents and inventory:

    val contents: Items = Items()
    private val inventory: Items = Items()

Now I expect many things to break, because Items doesn’t understand anything. Test to find out. The compiler will flag them. I’ll list each one and show you what I try. I’m going to try to get these working ASAP, but the Kotlin by thing is new to me.

    fun itemString(): String {
        return contents.values.joinToString(separator = "") {"You find ${it.shortDesc}.\n"}
    }

contents is an Items so it doesn’t know values.

class Items(map: ItemMap = mutableMapOf<String, Item>()) {
    val values by map
}

I think that should mean that you can say values to an Items and get back the values from the map (the one in the parameter list). I’ll test again to find out.

Expletive! The by trick doesn’t work. Belay that, we’ll do it the old-fashioned way. I’ll compile and deal with the errors by hand. Still should be easy enough.

class Room
    fun itemString(): String {
        return contents.values.joinToString(separator = "") {"You find ${it.shortDesc}.\n"}
    }

Seems like the easy thing would be to implement values to return the values:

class Items(map: ItemMap = mutableMapOf<String, Item>()) {
    val values = map.values
}

Test. New error:

    val take = { imperative: Imperative, world: World ->
        val item = contents.remove(imperative.noun)
        if ( item!=null ) {
            world.addToInventory(item)
            world.response.say("${item.name} taken.")
        } else {
            world.response.say("I see no ${imperative.noun} here!")
        }
    }

Doesn’t understand the remove. Right:

class Items(private val map: ItemMap = mutableMapOf<String, Item>()) {
    val values = map.values

    fun remove(key:String): Item? {
        return map.remove(key)
    }
}

Test. Find this:

    fun item(thing: String, details: Item.()->Unit): Item {
        val item = Item(thing)
        contents[thing] = item
        item.details()
        return item
    }

This code is tightly bound to the specifics of the Map. I already had to change it yesterday, because yesterday it was bound to the specifics of Set. What we want, I think, is to add an item to contents:

    fun item(thing: String, details: Item.()->Unit): Item {
        val item = Item(thing)
        contents.add(item)
        item.details()
        return item
    }

Kotlin and IDEA tell me to implement that and offer help.

    fun add(item: Item) {
        map[item.name] = item
    }

Test. Over in World:

    fun addToInventory(item:Item) {
        inventory[item.name] = item
    }

Easy change:

    fun addToInventory(item:Item) {
        inventory.add(item)
    }

Test.

    fun inventoryHas(item: String): Boolean {
        return inventory.contains(item)
    }

No contains method yet:

    fun contains(item: String): Boolean {
        return map.containsKey(item)
    }

Test. In a test:

        val item: Item = myRoom.contents["axe"]!!
        assertThat(item.shortDesc).isEqualTo("an axe")
        assertThat(item.longDesc).contains("Bridget Ingridsdotter")

I don’t like that this had to expect a null, but that’s the way maps are. I’d like to improve that but we’re trying to get back to green. Let’s do this:

        val item: Item = myRoom.contents.getItem("axe")!!

    fun getItem(name: String): Item? {
        return map[name]
    }

Test. Also in tests:

        assertThat(room.contents).containsKey("broom")
        assertThat(room.contents.size).isEqualTo(3)

We have Items.contains. We can use it here, I think.

        assertThat(room.contents.contains("broom")).isEqualTo(true)

We need to deal with size as well, but one thing at a time. That test passes. Now:

class Items(private val map: ItemMap = mutableMapOf<String, Item>()) {
    val size = map.size

Test. For some reason, I have to do this:

    fun size() = map.keys.size

There’s something I don’t understand about Kotlin properties vs functions. My other property works dynamically, this one did not. I have an inquiry out to my local experts.

Tests are green. Commit: Items are now kept in an Items instance, which contains whatever structure is needed (mutableMap just now).

I think there’s more that we might want to do, but let’s look now at what we’ve got. Here’s Items:

typealias ItemMap = MutableMap<String, Item>

class Items(private val map: ItemMap = mutableMapOf<String, Item>()) {
    fun size() = map.keys.size
    val values = map.values

    fun add(item: Item) {
        map[item.name] = item
    }

    fun contains(item: String): Boolean {
        return map.containsKey(item)
    }

    fun getItem(name: String): Item? {
        return map[name]
    }

    fun remove(key:String): Item? {
        return map.remove(key)
    }
}

This is all that you can do to an Items instance. When we had a mutableMap in our contents and inventory, there was access to all the millions of methods that maps support. That might be seen as an advantage, but I think it’s a disadvantage.

When I switched from Set to Map yesterday, I had to change several of those methods we looked at this morning, to use map approaches instead of Set approaches. And they were all over the application and tests. Now, if I want to change how Items are collected, my changes are all in only one place. I’ve just exposed a very simple object, a thing that contains instances of Item, and can access them by their name, a String.

You can add, remove, get, ask about whether something is there … and that’s almost all. The other thing you can do is get a collection of all the Item instances that are in there. That’s this, here:

class Items(private val map: ItemMap = mutableMapOf<String, Item>()) {
	...
    val values = map.values

And that’s not really very good. What we probably should have is a method named something like allItems or items. And I’d rather it was a function: this val property thing is confusing me and until I understand Kotlin better, I think I’d like to stay away from them.

So let’s fix that right now. Using Command B, I find two methods using values:

    fun itemString(): String {
        return contents.values.joinToString(separator = "") {"You find ${it.shortDesc}.\n"}
    }

    private fun showInventory() {
        say( inventory.values.joinToString(prefix="You have ", transform={it.shortDesc}, separator=", ", postfix=".\n") )
    }

Those are interesting, aren’t they? Very similar, one in the Room for what you find, and one in World, for what you have.

These two methods know a lot about what an Item looks like and also know that we can somehow produce a collection of them. I think they shouldn’t really need to know so much. Let’s see about moving the knowledge inside Items and the Item.

First, let’s give the Item a bit of intelligence, to answer asFound():

class Item(val name: String) {
    fun asFound() = "You find $shortDesc.\n"

Then we can change this:

    fun itemString(): String {
        return contents.values.joinToString(separator = "") {"You find ${it.shortDesc}.\n"}
    }

To this:

    fun itemString(): String {
        return contents.values.joinToString(separator = "") {it.asFound()}
    }

Test, of course. Green. Commit: Item understands asFound() returning useful room contents string.

But we can do more, with the help of Items:

    fun asFound(): String {
        return map.values.joinToString(separator = "") { it.asFound() }
    }

And now:

    fun itemString(): String {
        return contents.asFound()
    }

Test of course. Green. Commit: Items asFound returns “You find” list of all items.

I’ve removed one reference to Items.values. There’s another.


    private fun showInventory() {
        say( inventory.values.joinToString(
        	prefix="You have ", 
        	transform={it.shortDesc}, 
        	separator=", ", 
        	postfix=".\n") )
    }

This is, of course, feature envy. We’re pulling information out of inventory in order to process it. We should ask inventory for what we want. I think IDEA could have helped me with this but I can’t find the trick if there is one. I’ll do it manually, as it is trivial anyway:

    private fun showInventory() {
        say( inventory.asCarried() )
    }

And:

    fun asCarried(): String {
        return map.values.joinToString(prefix="You have ", transform={it.shortDesc}, separator=", ", postfix=".\n")
    }

We’re green. We can commit: Commit: Items.asCarried() returns standard inventory contents message.

But now we can remove the values return from Items, as it is no longer used. The entire class is now:

class Items(private val map: ItemMap = mutableMapOf<String, Item>()) {
    fun size() = map.keys.size

    fun add(item: Item) {
        map[item.name] = item
    }

    fun asCarried(): String {
        return map.values.joinToString(
            prefix = "You have ",
            transform = { it.shortDesc },
            separator = ", ",
            postfix = ".\n"
        )
    }

    fun asFound(): String {
        return map.values.joinToString(separator = "") { it.asFound() }
    }

    fun contains(item: String): Boolean {
        return map.containsKey(item)
    }

    fun getItem(name: String): Item? {
        return map[name]
    }

    fun remove(key:String): Item? {
        return map.remove(key)
    }
}

Very short and sweet, and now no other object knows the secrets of Items. It’s used in two places, which are quite similar in purpose. In Rooms it holds the instances of Item that are in the room, and in World, the ones that are in the player’s inventory. (We could argue that that belongs in Player, but that’s another issue.)

We used to have code in two main classes and several tests that was all dependent on the exact structure of contents and inventory, and now they no longer have to know that. They have become simpler, because we moved all the manipulation of the items inside the Items class (and the Item itself.)

Here are all the methods that refer to contents or inventory now:

// Room
    fun item(thing: String, details: Item.()->Unit): Item {
        val item = Item(thing)
        contents.add(item)
        item.details()
        return item
    }
    // no longer has to know how to do an add

    fun itemString(): String {
        return contents.asFound()
    }
    // no longer has to do detailed formatting

    val take = { imperative: Imperative, world: World ->
        val item = contents.remove(imperative.noun)
        ...
    // no longer has to know how to remove

// World
    fun addToInventory(item:Item) {
        inventory.add(item)
    }
    // no longer has to know how to add

    fun inventoryHas(item: String): Boolean {
        return inventory.contains(item)
    }
    // no longer has to know format of inventory

    private fun showInventory() {
        say( inventory.asCarried() )
    }
    // no longer concerned with details of
    // access and formatting

There are tests, as well, most of which are also made easier with he help of the Items collection.

My Take

My take is that moving from naked MutableMaps to my own Items object has simplified the code that uses them. That code used to have to think about Maps (and previously, Sets) and now it just thinks, OK, Items, ask it what I want to know.

Furthermore, everything that know about the Maps was spread around the system, in at least two main files and three or more test files. Now the details are all in one class, Items. I can change the way that class keeps track of things any time I want. When I want to change the formatting or any behavior, there’s just one class to look at.

I think this is a win. In my experience, this simplification always happens when I enclose a native collection in an object of my own. And I do mean “always”, or at least always minus epsilon.

So, in a brief morning’s work, and a half-dozen tiny commits, I’ve made the code just a bit better.

For me, that’s a win, and that’s why I think I’d do better if I just went ahead and created my own collections right away. I don’t do that: I’m lazy and not very bright. But when I do get around to it, it generally makes me happy.

What should you do? You should do as you choose, using judgment in the light of experience. You can borrow some of my judgment and experience if you wish … or not.

See you next time!