I think this morning’s thinking was good enough to deserve a chance. Could my thinking be wrong? It has happened before.

I like what this morning’s code does, in terms of searching for the best match for a phrase in a map of phrases. I don’t love how the code does it, but now that I see what it has to do, I can see a better way.

Let’s change our Actions object and see how things go. Unfortunately, I think this spells the end for the SmartMap. It contributed to our learning, but it doesn’t spark joy, so we’ll thank it for its service and let it go. Besides, we have Git. We can always “git” it back.

Here’s Actions:

typealias ActionMap = MutableMap<String, Action>

class Actions(map: ActionMap) {
    private val verbMap = SmartMap(map)

    fun act(imperative: Imperative) = verbMap.getValue((imperative.verb))(imperative)

    fun defineLocalActions(actions: ActionMap) {
        verbMap.clearLocal()
        verbMap.putAllLocal(actions)
    }

    fun putGlobal(action: Pair<String, Action>) = verbMap.putGlobal(action.first, action.second)
}

Now part of today’s idea is not to wrap up world actions and room actions into one ActionMap, but to keep them separate and deal with the two sources of action separately. Currently the code for command is here:

    fun command(command: Command, world: World) {
        world.response.nextRoomName = roomName
        world.defineLocalActions(actions)
        val factory = ImperativeFactory(world.lexicon)
        val imperative = factory.fromString(command.input)
        world.testVerb = imperative.verb
        world.testNoun = imperative.noun
        val imp = Imperative(imperative.verb,imperative.noun,world, this)
        imp.act(world.lexicon)
    }

In the call to defineLocalActions, we copy the room’s actions up to the world, and then use them down in imp.act where we look up the actions and execute them.

As I look at this now, I’m calling into question whether the actions should even be part of the Lexicon at all.

A Big Refactoring
I’m getting that “big refactoring” feeling. I feel like the design here is messed up, and that I don’t even see quite what it should be. This makes me feel that improving the situation is a big deal, and that I should back away slowly, because it’ll be a big set of changes to get where we need to go. Maybe it’s better to just make improvements around the edges, where we can probably do some things without too much trouble.

It is my firm belief that there is always a way to make the design what we want it to be in small steps. Well, that’s not quite my firm belief. My firm belief is that it is always better to assume that there is a way to get there in small steps, because if we don’t look for it, we’ll never find it, and so we’ll back away slowly, leaving messes to get worse. So let’s look around a bit more and see where there’s a thread to tug on to untangle this and make it better.

We might not find it, but we usually do. We might not get it the first time, but trying the first time will prepare us better for the next time.

Let’s follow the feeling that act perhaps belongs outside the Lexicon, and take a look at how it works now.

class Imperative ...
    fun act(lexicon: Lexicon): String {
        lexicon.act(this)
        return testingSaid
    }

class Lexicon ...
    fun act(imperative: Imperative) = actions.act(imperative)

class Actions(map: ActionMap) {
    private val verbMap = SmartMap(map)
    fun act(imperative: Imperative) = verbMap.getValue((imperative.verb))(imperative)

I think we “know” that we want our ActionMap to be a map from Phrase to Action. Right now, it’s just a typealias, not a real class. If we were to redefine that type, Kotlin will tell us what we need to change to make things work. We can try it and see how bad that looks. We’re at a save point.

I start from here:

data class Phrase(val verb: String?=null, val noun: String?=null)
typealias Action = (Imperative) -> Unit
typealias ActionMap = MutableMap<String, Action>

And change the final typealias:

data class Phrase(val verb: String?=null, val noun: String?=null)
typealias Action = (Imperative) -> Unit
typealias ActionMap = MutableMap<Phrase, Action>

IDEA’s Problems window points me to two problems, both saying

Type mismatch: inferred type is String but Phrase was expected

The two lines in question are:

class Actions ...
    fun act(imperative: Imperative) = verbMap.getValue((imperative.verb))(imperative)

    fun putGlobal(action: Pair<String, Action>) = verbMap.putGlobal(action.first, action.second)

I think that, for now, we should be able to fix both of these in place.

    fun act(imperative: Imperative) = verbMap.getValue(Phrase(imperative.verb))(imperative)

    fun putGlobal(action: Pair<String, Action>) = verbMap.putGlobal(Phrase(action.first), action.second)

Let’s see what the tests say.

Type mismatch: inferred type is MutableMap<String, (Imperative) -> Unit> but ActionMap /* = MutableMap<Phrase, Action /* = (Imperative) -> Unit */> */ was expected

That’s from this:

    private fun makeActions(): Actions {
        return Actions(mutableMapOf(
            "go" to { imp: Imperative -> imp.room.move(imp, imp.world) },
            "say" to { imp: Imperative -> imp.room.castSpell(imp, imp.world) },
            "take" to { imp: Imperative -> imp.room.take(imp, imp.world) },
            "inventory" to { imp: Imperative -> imp.world.showInventory() },
//            "shout" to { imp: Imperative -> imp.say(
//                "Your shout of ${imp.noun.uppercase()} echoes through the area.")},
        ).withDefault {
            { imp: Imperative -> imp.room.unknown(imp, imp.world) }
        }

This is starting to get messy: this is a third statement that I’ll have to change. Don’t give up yet: I think it just wants all those strings to be Phrases.

    private fun makeActions(): Actions {
        return Actions(mutableMapOf(
            Phrase("go") to { imp: Imperative -> imp.room.move(imp, imp.world) },
            Phrase("say") to { imp: Imperative -> imp.room.castSpell(imp, imp.world) },
            Phrase("take") to { imp: Imperative -> imp.room.take(imp, imp.world) },
            Phrase("inventory") to { imp: Imperative -> imp.world.showInventory() },
//            Phrase("shout") to { imp: Imperative -> imp.say(
//                "Your shout of ${imp.noun.uppercase()} echoes through the area.")},
        ).withDefault {
            { imp: Imperative -> imp.room.unknown(imp, imp.world) }
        }

I’m getting really good with that multi-cursor edit thing. Test.

Type mismatch: inferred type is MutableMap<String, (Imperative) -> Unit> but ActionMap /* = MutableMap<Phrase, Action /* = (Imperative) -> Unit */> */ was expected

This time it’s this line:

class Room ...
    fun command(command: Command, world: World) {
        world.response.nextRoomName = roomName
        world.defineLocalActions(actions)

I am tempted to comment that out and just not pass in any local commands, but I think the real fix will be easy as well. For now, let’s do comment it out, to see what happens next.

We get the type message on the test table:

    private fun getActions() = Actions(TestActionTable)

private val TestActionTable = mutableMapOf(
    "go" to { imp: Imperative -> imp.testingSay("went ${imp.noun}")},
    "say" to { imp: Imperative -> imp.testingSay("said ${imp.noun}")},
    "inventory" to { imp: Imperative -> imp.testingSay("You got nothing")}
).withDefault { { imp: Imperative -> imp.testingSay("I can't ${imp.verb} a ${imp.noun}") }}

Fix like the other one:

private val TestActionTable = mutableMapOf(
    Phrase("go") to { imp: Imperative -> imp.testingSay("went ${imp.noun}")},
    Phrase("say") to { imp: Imperative -> imp.testingSay("said ${imp.noun}")},
    Phrase("inventory") to { imp: Imperative -> imp.testingSay("You got nothing")}
).withDefault { { imp: Imperative -> imp.testingSay("I can't ${imp.verb} a ${imp.noun}") }}

Test. And we are green! We’ve converted everything to use the Phrase in the lookup. (I’m a bit disappointed that not even one test failed: there may not be a decent test for the local actions that we commented out, but that has to change anyway.)

We can take a save point. Commit: Convert ActionMap to <Phrase,Action>.

The commit warns me that defineLocalActions is never used, and I think we should get rid of it, and the commented-out call to it, because we aren’t going to keep that double-level SmartMap anyway. Let’s do that next but first:

Reflection
Less than an hour, including all this writing, and we have a key change in place, the use of the Phrase in the action lookup maps, which is important to our use of the matching wild card search.

Let’s pop over to that defineLocalActions:

class World ...
    fun defineLocalActions(actions: ActionMap) = lexicon.defineLocalActions(actions)

class Lexicon ...
    fun defineLocalActions(newActions: ActionMap) = actions.defineLocalActions(newActions)

class Actions ... 
    fun defineLocalActions(actions: ActionMap) {
        verbMap.clearLocal()
        verbMap.putAllLocal(actions)
    }


class SmartMap<K,V>( ...
    fun clearLocal() = local.clear()
    fun putAllLocal(actions: MutableMap<K, V>) = local.putAll(actions)

We’ll doubtless be able to clear this all the way down. I think if we start at the top, IDEA’s Problems window may just track us right through it. Let’s try it, I’m here to learn this tool set.

I remove the one in World, and the commented out call in Room. I guess I have to try the tests or something so that it’ll know the problems. Maybe that Analyze thing?

The command I find is Code / Inspect Code ...

It tells me about the method in Lexicon. I remove it. That wasn’t as helpful as it might be in tracking through the unused things. I think there are some interesting items under the Code menu but I’m impatient. Let’s just run the tests: We get better warnings that way.

That points me to the one in Lexicon. I remove that. No more warnings.

I think I’ve just changed ActionMap so that it no longer needs to contain that two-level SmartMap:

class Actions(map: ActionMap) {
    private val verbMap = SmartMap(map)
    fun act(imperative: Imperative) = verbMap.getValue(Phrase(imperative.verb))(imperative)
    fun putGlobal(action: Pair<String, Action>) = verbMap.putGlobal(Phrase(action.first), action.second)
}

Can I just remove verbMap and have this object refer to the map? My change breaks a test. I roll back. OK, Green again. The test was this one:

    @Test
    fun `shouts echo`() {
        val world = world {
            room("woods") {
                desc("You are in the woods.", "You are in the dark woods.")
            }
        }
        val room = world.unsafeRoomNamed("woods")
        val response = GameResponse()
        val command = Command("shout hello")
        world.command(command, room, response)
        assertThat(response.resultString).contains("Your shout of HELLO echoes through the area.")
    }

That relies on this, which was a bit tricky to discover, took me a moment:

class World ...
    fun addShout() {
        lexicon.actions.putGlobal("shout" to { imp: Imperative -> imp.say(
            "Your shout of ${imp.noun.uppercase()} echoes through the area.")},)
    }

Where is that done?

fun world(details: World.()->Unit): World{
    val world = World()
    world.addShout()
    world.details()
    return world
}

That’s an early experiment for adding new commands dynamically. Let’s trust ourselves to work on that, and, for now, remove that code and test.

Commit: Remove test and support code for shout, to be re-added later.

Now let’s go after the SmartMap in Actions again.

class Actions(map: ActionMap) {
    private val verbMap = SmartMap(map)
    fun act(imperative: Imperative) = verbMap.getValue(Phrase(imperative.verb))(imperative)
    fun putGlobal(action: Pair<String, Action>) = verbMap.putGlobal(Phrase(action.first), action.second)
}

The putGlobal is now unused, according to IDEA. Remove, test. Green. Remove the SmartMap reference:

class Actions(val map: ActionMap) {
    fun act(imperative: Imperative) = map.getValue(Phrase(imperative.verb))(imperative)
}

That class is pretty light-weight. May not be carrying its weight. We’ll think about that. Test. Green. Commit: Remove SmartMap from Actions.

Now can we perhaps get rid of the SmartMap altogether?

All the references are in tests. Let’s remove the lot. We can get it back if we want it, and we won’t want it.

Deleted. Green. Commit:

If I’m reading the Git log correctly, we’ve deleted 178 lines and everything is still working except the shout command. We could put that back if we really wanted it, but it was just a proof of concept for adding to the actions on the fly, and we’ll do that differently, from the DSL, in due time.

The “big change” is that the actions dictionary in the Lexicon is now indexed by Phrase, not just String, so that we can use our smart search on it.

However, to date we only need to search for verbs, so there’s no need to plug in the smart search yet. Still, I’d like to “document” my idea for how to do it. There’s already a bad way in PhraseMapTest, let’s do a good one. Here’s what we have now:

    @Test
    fun `priority match`() {
        val map: Map<Phrase,String> = makeMap()
        val result:String = find(Phrase("take","cows"), map)
        assertThat(result).isEqualTo("takecows")
    }

    @Test
    fun `take non-cows`() {
        val map = makeMap()
        val result = find(Phrase("take", "bird"), map)
        assertThat(result).isEqualTo("take")
    }

    @Test
    fun `something with cows`() {
        val map = makeMap()
        val result = find(Phrase( noun="cows"), map)
        assertThat(result).isEqualTo("cows")
    }

    @Test
    fun `something different`() {
        val map = makeMap()
        val result = find(Phrase("something", "different"), map)
        assertThat(result).isEqualTo("any")
    }

    private fun makeMap() = mapOf(
        Phrase("take", "cows") to "takecows",
        Phrase("take") to "take",
        Phrase(noun = "cows") to "cows",
        Phrase() to "any"
    )

    private fun find(p:Phrase, m:Map<Phrase,String>): String {
        val p2 = Phrase(p.verb)
        val p3 = Phrase(noun=p.noun)
        val p4 = Phrase()
        var results = listOf(p,p2,p3,p4).map {m.getOrDefault(it,"nope")}
        print("$p->")
        print("$results->")
        results = results.filter { it != "nope" }
        println("$results")
        return results.first()
    }

I’ll rename find to oldFind and do the new one.

    private fun find(p:Phrase, m: Map<Phrase, String>): String {
        return m.getOrElse(p) {
            m.getOrElse(Phrase(p.verb)) {
                m.getOrElse(Phrase(noun=p.noun)) {
                    m.getOrElse(Phrase()) {"nothing"}
                }
            }
        }
    }

That passes the test just fine and makes more sense to me. I think that’s rather clean and nice. Let’s commit: Better find in PhraseMapTest.

I think the Phrase could be helping out.

data class Phrase(val verb: String?=null, val noun: String?=null) {
    fun asVerb() = Phrase(this.verb)
    fun asNoun() = Phrase(noun=this.noun)
    fun asEmpty() = Phrase()
}

    private fun find(p:Phrase, m: Map<Phrase, String>): String {
        return m.getOrElse(p) {
            m.getOrElse(p.asVerb()) {
                m.getOrElse(p.asNoun()) {
                    m.getOrElse(p.asEmpty()) {"nothing"}
                }
            }
        }
    }

That’s a bit nicer, I think. Commit: Phrase has asVerb, asNoun, asEmpty.

Let’s sum up.

Summary

Note in Particular
Despite what felt like a big refactoring coming on, starting in the right place allowed us to change how the basic objects worked, to remove long threads of code guided by IDEA and our tests, and so far, no “big” steps have been needed. I think that’s always the way to bet: that we can find a step-by-step way.

We’re removed all, or at least most, of the SmartMap and its vestiges. We’ve improved Phrase, and set up Actions class to have a more robust matching search which I (still speculatively) expect to want. We did it in very small steps, With 8 commits over two hours, and a net removal of around 175 lines of code with no lost of function in the game and only one experimental test needing removal.

We’ve got a rather nice matching search algorithm that we can put into Actions, and we should be well set up to add actions to rooms. the new plan for that is just to keep them in the room and refer to them first when we get a command, which comes to the room anyway.

We probably have a bit too much stuff going on with Lexicon (which probably shouldn’t contain the world’s Actions, since it won’t contain the Room’s). I’m not sure about the Imperative vs the Phrase. Possibly the Imperative should contain a Phrase now, since we’re going to convert to one anyway to search for the command.

In all, I think we’re in a better place, and positioned to make things even simpler. I am satisfied with the afternoon’s work.

See you next time!