OK, with that off my chest, we can do some code. Let’s keep after Actions and the associated items. Things do not go as I expect.

As I sort of recall, yesterday I wrote a test for this rather nice way of searching for Actions. The idea is to find the first one that matches, in the order verb+noun match, just verb, just noun, then anything matches. The code is free-standing in the test file for now:

    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"}
                }
            }
        }
    }

The thing, I think, is to map this into our current scheme for finding the actions. That’s done in Room.command:

    fun command(command: Command, world: World) {
        world.response.nextRoomName = roomName
        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)
    }

(I’s about time that I got rid of those two test-related lines in there. Perhaps today.)

The code above is weird, reflecting a bit of awkwardness in how things are set up. Given an Imperative, we ask it to act, passing it a lexicon. But in order for that to work, we need access to the world and the room. That’s packed into the Imperative, because the Imperative is passed to the Action lambda, because the lambda action needs access. So … the code above recreates the Imperative at the last minute, passing in the correct world and this room.

I’m not proud of that. We could pass world and room separately. Instead of an imperative looking like this:

{ imp: Imperative -> imp.room.move(imp, imp.world) }

It could look like this:

{ imp, world,room -> room.move(imp, world)}

Perhaps even something better than that. I think the Imperative has served its purpose, and should probably be replaced. The actions need to know four things, and no more, I believe. They need the world, the room, the verb, and the noun. Or, in present parlance, the world, the room, and the phrase. We’ll see whether we can get there.

There is some advantage to the action lambda having only one parameter: people will be typing a lot of them. On the other hand, and the other side of the arrow ->, they’ll have to dereference a single parameter all the time, which is also awkward. We’ll see what we see.

The trick, today as every day, is to find very small steps to take, not breaking much, to get from here to a better place. Our tests, plus IDEA and Kotlin will help us find what would break and we’ll fix it back up. Naturally, when we’re half-way through a change or refactoring, things are broken. The trick is to make that broken period as short as possible, and the broken bits few and easy to find.

Let’s see how the Imperative does its act method: that seems like a nice central place to insert our Phrase idea.

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

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

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

Ah. We have a design question to ask ourselves here. In the code above, we’re relying on the map to do an intelligent kind of getValue, returning a default and whatnot. Originally, that map was a SmartMap, but we removed that class yesterday. Anyway, we want our more clever find to be applied, so ideally we’d create an object, or perhaps extend Map, to provide our find.

But what is this map? Aren’t we already putting Phrases in there? Yes, we are:

    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) }
        }
        )
    }
You may be asking yourself
Why do I take us through all this discovery, instead of cutting to the chase and saying “OK, we’re gonna change this now”? I do it this way because in real life, what we need to do in the code isn’t handed to us on a platter. We have to study and rediscover the code, to find the right places to put our changes. The fact is that I don’t know in some magical way just what to do: I read the code and figure it out. Possibly you have to read code to figure out what to do as well.

This does suggest writing code as if it were intended to be read more often than written, doesn’t it? Hmmm …

Anyway, if we were to pass in the full phrase here:

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

The current search would fail, because it’s doing an exact match. Let’s try it:

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

I expect this to fail all over. 13 tests out of 36 fail. The messages all look something like this:

Expecting:
 <"I can't go a east">
to be equal to:
 <"went east">
but was not.

Right. Let’s see if I can extend MutableMap to have a find.

Arrgh!
I’ve now spent too long trying to get an extension to the map to compile. Wasting time. I’ll see if I can’t just plug my find into the act method instead.

After more faffing about, I realize that my function act actually calls the returned Action. It should just be a moment or two to make this work. Time is 0928.

No still fouled up. Let me revert and do again. This morning is an example of why even the best practices I know don’t always work miracles.

OK, that didn’t work. Let’s proceed in even smaller steps. We’re trying to plug my new cascaded find algorithm into this:

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

Class has only one method, how hard could it be. First, extract a variable.

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

Extract the lookup into a separate method:

class Actions(val map: ActionMap) {
    fun act(imperative: Imperative) {
        val imp: (Imperative) -> Unit = find(imperative)
        imp(imperative)
    }

    private fun find(imperative: Imperative) = map.getValue(Phrase(imperative.verb))
}

Make that function wrap in braces (convert to block body):

    private fun find(imperative: Imperative): Action {
        return map.getValue(Phrase(imperative.verb))
    }

Pull out the embedded creation of the phrase:

    private fun find(imperative: Imperative): Action {
        val p = Phrase(imperative.verb)
        return map.getValue(p)
    }

We’re going to need to change that phrase when we put in our find. I’m going to try to type in the find one bit at a time instead of pasting the whole thing. I was getting objections from Kotlin about the types.

Oh. I just realized that as written, our map has a default:

    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) }
        }
        )
    }

That’s going to mess up my find, which needs to fail in order to succeed. Let’s remove the default and fix it in the current find.

I mess around a while with trouble, then back up to what was above and grab a save point commit. I’m green.

Arrgh Again!

What I tried was to remove that default, and instead provide it in the find. It made sense to me. Let me do it again right here in front of us and see what I’m getting wrong.

First comment out the default:

    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() },
        )//.withDefault {
//            { imp: Imperative -> imp.room.unknown(imp, imp.world) }
//        }
        )
    }

Small steps. Test expecting a fail with no default.

Key Phrase(verb=abcd, noun=null) is missing in the map.

Right. So we should be able to supply it in the find. I have this in find (rolled back a bit, remember):

    private fun find(imperative: Imperative): Action {
        return map.getValue(Phrase(imperative.verb))
    }

I want a getOrDefault here, I guess, and it seems I could return the same value as the withDefault:

    private fun find(imperative: Imperative): Action {
        val default = { imp: Imperative -> imp.room.unknown(imp, imp.world) }
        return map.getOrDefault(Phrase(imperative.verb), default)
    }

I think this is what I did last time and I got an error. Test. Yes, same thing, two occurrences of this error:

lateinit property response has not been initialized

I bet I have to pass in values from my input Imperative. But why does it work in the other location? I don’t see the difference, but I think the issue is more with the test setup than with the code we’re working on.

Attack of conscience …

What would be best would be to figure out why these tests have started to fail: there must be some difference between the thing that comes back from the default and mine. But how the heck can you inspect a lambda?

Let’s save my new one, leave the default in, and compare them.

Leads to better behavior

As I feared, they can’t compare. Let’s write a simple test that works in the one mode and fails in the other and see what we can learn. I am definitely confused about how this can possibly not be the same.

    @Test
    fun `one failing lookup`() {
        val imperatives  = getFactory()
        val imp = Imperative("forge", "sword", world, room)
        assertThat(imp.act(testLex())).isEqualTo("I can't forge a sword")
    }

That passes. Now the change to remove the default from the table should break it.

Sudden realization

Oh! The tests use a different table! I’ve not been changing that one.

Better change at least that one for this test.

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}") }}

Oh my! This can’t work. We need a different default here. Anyway let’s plug that one in for our default and see if we can get sorted. I think we’ll need to provide the default more dynamically since it’s not in the table any more …

Unless … maybe we can do something. Main thing now is to learn how to do it at all.

    private fun find(imperative: Imperative): Action {
//        val default = { imp: Imperative 
//			-> imp.room.unknown(imp, imp.world) }
        val default = { imp: Imperative 
        	-> imp.testingSay("I can't ${imp.verb} a ${imp.noun}") }
        val ret = map.getOrDefault(
        	Phrase(imperative.verb), default)
        return ret
    }

Now that I have the right default, I have high hopes for my new test and the other two that failed. My new one passes. Let’s try that whole group. They all pass. But I bet some others fail for want of the right default. Run the whole suite.

Yes, one other one fails for me:

Expecting:
 <"You are in the dark woods.
">
to contain:
 <"unknown command 'abcd efgh'">

I bet if I set the default the other way this one will pass. It does.

Clarify the cause,
clarity is useful.

All this confusion has been due to the fact that I use special defaults in the tests, and had forgotten it, so that weird things happen.

I have an idea for a hack that can get us moving forward: Suppose we put the default in the table, with a special key, “:default:”. Then we’ll look for that and return it when the other result isn’t there.

That change may not be righteous, though it has some art to it. In fact, if we put it in there at the key of an empty Phrase, it’s probably what we want anyway.

Let’s do that in both action setups.

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")},
    Phrase() to { imp: Imperative 
    	-> imp.testingSay("I can't ${imp.verb} a ${imp.noun}") }
)//.withDefault { { imp: Imperative -> imp.testingSay("I can't ${imp.verb} a ${imp.noun}") }}

    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() to { imp: Imperative 
            	-> imp.room.unknown(imp, imp.world) }
        )//.withDefault {
//            { imp: Imperative -> imp.room.unknown(imp, imp.world) }
//        }
        )
    }

Now in the find we have to fetch that value:

    private fun find(imperative: Imperative): Action {
        val default = { imp: Imperative 
        	-> imp.room.unknown(imp, imp.world) }
//        val default = { imp: Imperative -> imp.testingSay("I can't ${imp.verb} a ${imp.noun}") }
        val ret = map.getOrElse(Phrase(imperative.verb)) { 
        	map.getValue(Phrase())}
        return ret
    }
}

I rather expect this to run all my tests. And it does. Commit: actions find save point.

I can clean up the find method a bit, then I want a break, after some reflection.

    private fun find(imperative: Imperative): Action {
        return map.getOrElse(Phrase(imperative.verb)) { 
        	map.getValue(Phrase())}
    }

Test. Green. Commit: tidying. Let’s reflect, then I want a break: it’s 1045 and I’ve been here since 0815, and actually started the previous article at 0745.

Reflection
I’ve spent much of the past few hours thinking and fumbling, occasioned by pasting in that long find method from my test into the live code and trying to get it to compile. I still don’t know why it wouldn’t, but I have a better place to stand now, and I plan to put it in one getOrElse at a time. I think that’ll make it easier.

But then, trying to go in smaller steps. I got confused by failures that seemed impossible. At first, I had forgotten that my smart maps never fail to return something, so my getOrElse was doomed even if I did build it up bit by bit. And then I forgot that my test tables and live tables have different defaults.

So my thoughts, it turns out, were much more messy even than the article suggests.

Is there a lesson for me here? Well … my tests using different data from place to place wasn’t a help. But it seems to me that sometimes you really need to do that, because the tests want to look inside more deeply, so that the live code can go about its business without drilling in. Still, we see that it was an issue.

The data for the tests is kind of hidden away, and since the tables include default behavior, that caused confusion. I can imagine that the withDefault table property could do that. You set up a default, then later on something works differently than you expect. That probably gets less likely to cause a problem when you’re more used to the feature. But I want to kind of yellow-flag it.

I thought I was going in pretty small steps all along, but when the going got touch, I started making very small changes, extract this to a variable, extract that to a function, and so on. IDEA/Kotlin helped me with that quite a bit. I’m gaining facility with this powerful tool set.

As I gain facility, I think that will pay off more and more, because it makes the cost of small steps lower.

We’ll wrap this here and come back, this afternoon, or tomorrow morning, as the fancy strikes me. See you then!