Kotlin 65: Tidying

Some tidying, but I do need to find a smooth way to do it.

Thing One

I’d like to move the World’s Actions from the Lexicon to World, so as to be symmetric with Room, which has its own. It turns out there is at least one wrong way to do that.

I tried just changing the signature on the Lexicon, thinking that doing so would conveniently highlight the errors for me, so I removed the Actions from this:

class Lexicon(private val synonyms: Synonyms, private val verbs: Verbs, val actions: Actions) {
    fun synonym(word:String):String = synonyms.synonym(word)
    fun translate(word: String): Phrase = verbs.translate(word)
}

Unfortunately, the Change Signature refactoring just found all my construction calls and removed the actions from them. That wasn’t really what I had in mind. I could trek through the errors in compile and test but that doesn’t seem fun. Let’s see if there is a better way. Roll back.

Let’s see who creates Lexicons. Here they all are. I’ll look at all of them even though just a few might give me a decent idea.

In World, I have a private method, makeLexicon, which is called whenever you create the world. It is intended to be the “official” version of Lexicon in the class.

class Lexicon(private val synonyms: Synonyms, private val verbs: Verbs, val actions: Actions) {
    fun synonym(word:String):String = synonyms.synonym(word)
    fun translate(word: String): Phrase = verbs.translate(word)
}

In testing we create a special one:

    private fun testLex(): Lexicon {
        val synonyms = getSynonyms()
        val verbs = getVerbs()
        val actions = getActions()
        return Lexicon(synonyms, verbs, actions)
    }

Some early tests create their own:

class ImperativeTest

    @Test
    fun `create a lexicon`() {
        val synonyms = getSynonyms()
        val verbs = getVerbs()
        val actions = getActions()
        val lexicon = Lexicon(synonyms, verbs, actions)
        assertThat(lexicon.synonym("e")).isEqualTo("east")
        val phrase: Phrase = lexicon.translate(
            lexicon.synonym("e")
        )
        val imp = Imperative(phrase, world, room)
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
        assertThat(imp.actForTesting(lexicon)).isEqualTo("went east")
    }

  private fun getLexicon() = Lexicon(getSynonyms(),getVerbs(),getActions())
  ...

    private fun testLex(): Lexicon {
        val synonyms = getSynonyms()
        val verbs = getVerbs()
        val actions = getActions()
        return Lexicon(synonyms, verbs, actions)
    }

I think we need to see who’s using that testLex method, they’re going to be trouble.

Yes, there are 18 calls to that. Not terribly efficient either, since each call creates a lexicon. They look like this:

    @Test
    fun `imperative can act`() {
        val factory = getFactory()
        var imp = factory.fromOneWord("east").asImperative()
        assertThat(imp.actForTesting(testLex())).isEqualTo("went east")
        imp = factory.fromOneWord("e").asImperative()
        assertThat(imp.actForTesting(testLex())).isEqualTo("went east")
        imp = Imperative(Phrase("forge", "sword"), world, room)
        assertThat(imp.actForTesting(testLex())).isEqualTo("I can't forge a sword")
    }

    @Test
    fun `new phrase finding handles all cases`() {
        val imperatives  = getFactory()
        var imp: Imperative = imperatives.fromTwoWords("take", "cows").asImperative()
        assertThat(imp.actForTesting(testLex())).isEqualTo("no cows for you")
        imp = imperatives.fromTwoWords("hassle","cows").asImperative()
        assertThat(imp.actForTesting(testLex())).isEqualTo("please do not bug the cows")
        imp = imperatives.fromTwoWords("pet","cows").asImperative()
        assertThat(imp.actForTesting(testLex())).isEqualTo("what is it with you and cows?")
        imp = imperatives.fromTwoWords("hassle","bats").asImperative()
        assertThat(imp.actForTesting(testLex())).isEqualTo("please do not bug the bats")
        imp = Imperative(Phrase("forge", "sword"), world, room)
        assertThat(imp.actForTesting(testLex())).isEqualTo("I can't forge a sword")
    }

    @Test
    fun `one failing lookup`() {
        val imp = Imperative(Phrase("forge", "sword"), world, room)
        assertThat(imp.actForTesting(testLex())).isEqualTo("I can't forge a sword")
    }
// (And more.) 

Let’s do this. Let’s change actForTesting to expect an Actions instead of a Lexicon. Then … I have an idea for what to do next. If this doesn’t work, we’ll roll back and come at it from another angle.

    fun actForTesting(actions: Actions): String {
        act(actions)
        return testingSaid
    }

Then, in all those calls, we’ll change to testActions().

No. Another order is better. Let’s first build testActions()

    private fun testActions(): Actions {
        return testLex().actions
    }

Now we can change those calls one at a time if we want to: Here’s one:

    @Test
    fun `imperative can act`() {
        val factory = getFactory()
        var imp = factory.fromOneWord("east").asImperative()
        assertThat(imp.actForTesting(testLex())).isEqualTo("went east")
        imp = factory.fromOneWord("e").asImperative()
        assertThat(imp.actForTesting(testLex())).isEqualTo("went east")
        imp = Imperative(Phrase("forge", "sword"), world, room)
        assertThat(imp.actForTesting(testLex())).isEqualTo("I can't forge a sword")
    }

I’ll just change the first one, as this is a proof of concept:

    @Test
    fun `imperative can act`() {
        val factory = getFactory()
        var imp = factory.fromOneWord("east").asImperative()
        assertThat(imp.actForTesting(testActions())).isEqualTo("went east")
        imp = factory.fromOneWord("e").asImperative()
        assertThat(imp.actForTesting(testLex())).isEqualTo("went east")
        imp = Imperative(Phrase("forge", "sword"), world, room)
        assertThat(imp.actForTesting(testLex())).isEqualTo("I can't forge a sword")
    }

Test. Oops, forgot to make the other method:

    fun actForTesting(lexicon: Lexicon): String {
        act(lexicon.actions)
        return testingSaid
    }
    
    fun actForTesting(actions: Actions): String {
        act(actions)
        return testingSaid
    }

Now we have two possibilities. First make sure it works.

We are green. Forward from the one to the other:

    fun actForTesting(lexicon: Lexicon): String {
        return actForTesting(lexicon.actions)
    }

    fun actForTesting(actions: Actions): String {
        act(actions)
        return testingSaid
    }

We avoid a bit of duplication there, and I think it’s important to have everyone going through the second method just in case I mess something up.

Now let’s take a look at testLex():

    private fun testLex(): Lexicon {
        val synonyms = getSynonyms()
        val verbs = getVerbs()
        val actions = getActions()
        return Lexicon(synonyms, verbs, actions)
    }

By the way, I could commit right now with perfect safety.

Next step, let’s put a var on World, named actions.

class World {
    var actions: Actions = makeActions()
    val lexicon = makeLexicon()

This should run the tests just fine: we’ve just made a second copy of the Actions that go into makeLexicon. Yes, we’re green. Could commit. I guess I will, in fact, I think this is going well enough that we won’t give up.

Commit: World creation now initializes actions member variable.

Now let’s modify testLex to store the actions in world:

    private fun testLex(): Lexicon {
        val synonyms = getSynonyms()
        val verbs = getVerbs()
        val actions = getActions()
        world.actions = actions
        return Lexicon(synonyms, verbs, actions)
    }

Now they are in both locations (there is an instance of world in scope in the tests). First test as we stand … Green. Then change this to refer to world.actions rather than the lexicon:

    private fun testActions(): Actions {
        return world.actions
    }

Test.

At our convenience, over the course of weeks if necessary, replace the testLex calls with testActions. I wonder if there is a way for IDEA to help me with that.

I decide to multi-cursor a bunch at a time. Loaded copy buffer with testActions, select a testLex, do control-G to select a bunch and paste.

Oops! I find that I can’t just do this. This test fails:

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

The error is that response is not initialized. Let’s fix that once and for all:

class World ...
    var response: GameResponse = GameResponse()

That was formerly lateinit. Let’s see what breaks if we just create one out of the box.

Hmm, test still fails, now saying:

Expecting:
 <"">
to be equal to:
 <"I can't forge a sword">
but was not.

We got no response back. I have a theory that we may need to call testLex in any case. Yes. Changing the test to this:

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

Makes it work. This is part of our nasty initialization flow. How do I do a “before” in this test tool?

    @BeforeEach
    fun `before each`() {
        testLex()
    }

Right. Now let’s find and replace those testLex calls one at a time. We can commit now and after each one that works.

Many possible commits later, we are still green. Commit: All tests using textAction where applicable.

Let’s remember what we’re about: we don’t want the Lexicon to have actions in it any more. Let’s rename the testLex function, which is now only used in our @BeforeEach:

    private fun testSetUpLexAndActions(): Lexicon {
        val synonyms = getSynonyms()
        val verbs = getVerbs()
        val actions = getActions()
        world.actions = actions
        return Lexicon(synonyms, verbs, actions)
    }

We can of course Commit: rename test prep function.

Now let’s see all the Lexicon creators. There are surely only a couple.

class World ...
    private fun makeLexicon(): Lexicon {
        return Lexicon(makeSynonyms(), makeVerbs(), makeActions())
    }

    @Test
    fun `create a lexicon`() {
        val synonyms = getSynonyms()
        val verbs = getVerbs()
        val actions = getActions()
        val lexicon = Lexicon(synonyms, verbs, actions)
        assertThat(lexicon.synonym("e")).isEqualTo("east")
        val phrase: Phrase = lexicon.translate(
            lexicon.synonym("e")
        )
        val imp = Imperative(phrase, world, room)
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
        assertThat(imp.actForTesting(lexicon)).isEqualTo("went east")
    }

       private fun getLexicon() = Lexicon(getSynonyms(),getVerbs(),getActions())


    private fun testSetUpLexAndActions(): Lexicon {
        val synonyms = getSynonyms()
        val verbs = getVerbs()
        val actions = getActions()
        world.actions = actions
        return Lexicon(synonyms, verbs, actions)
    }

My plan, since I am on a fresh commit, is to change the signature now and fix the problems. If this is a bad plan, I’ll know in a moment.

Command-F6 and remove the actions parameter:

class Lexicon(private val synonyms: Synonyms, private val verbs: Verbs) {
    fun synonym(word:String):String = synonyms.synonym(word)
    fun translate(word: String): Phrase = verbs.translate(word)
}

Test to see what won’t go.

    fun actForTesting(lexicon: Lexicon): String {
        return actForTesting(lexicon.actions)
    }

Someone is still calling that. Find them.

    @Test
    fun `create a lexicon`() {
        val synonyms = getSynonyms()
        val verbs = getVerbs()
        val actions = getActions()
        val lexicon = Lexicon(synonyms, verbs)
        assertThat(lexicon.synonym("e")).isEqualTo("east")
        val phrase: Phrase = lexicon.translate(
            lexicon.synonym("e")
        )
        val imp = Imperative(phrase, world, room)
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
        assertThat(imp.actForTesting(lexicon)).isEqualTo("went east")
    }

Ah, yes. We can now say world.actions here, I think. Change it, safe delete the method. Command won’t compile:

    fun command(command: Command, world: World) {
        world.response.nextRoomName = roomName
        val phrase: Phrase = makePhrase(command, world.lexicon)
        val imp = Imperative(phrase,world, this)
        imp.act(actions, world.lexicon.actions)
    }

Change to:

    fun command(command: Command, world: World) {
        world.response.nextRoomName = roomName
        val phrase: Phrase = makePhrase(command, world.lexicon)
        val imp = Imperative(phrase,world, this)
        imp.act(actions, world.actions)
    }

I don’t remember whether this is initialized or not. We’ll find out.

We are green. Actions are moved from Lexicon to World, as intended.

Very small steps, hardly any missteps (except for some overly aggressive multi-cursor action, but I need practice).

Commit: Actions moved from Lexicon to World.

So this is nice. I do think there’s more to do. The world sets itself up with all the “real” lexicon and actions, and the tests kind of override that. We should probably have some kind of strategy approach to that. That would be a good learning experience for me in Kotlin.

And surely there are other things not to like. We’ll look for some tomorrow.

Join me then!