Kotlin 61: Untangling

GitHub Repo

Let’s see if we can untangle things a bit in and around Imperative, Phrase, and the like.

It’s 0500 hours and I am awake for some reason. I choose to use the time to improve the ADVENT program and to work on my Kotlin/IDEA skills. Since I’ve been working in the area of command processing, I have noticed the awkwardness that is present, especially in tests, mostly relating to Imperative, its Factory, and the command method. I plan to untangle the code in that area. I’ll be driven by what the code actually is, but I do have an impression of what’s wrong and what needs doing. I’ll share that first. It’ll perhaps be interesting to see how close my impression comes to what we actually find and do.

Impression

The Imperative object is strange. Its main purpose seems to be to carry the command that needs to be done (in the form of separate verb and noun), plus the world, and the room in which the command is to be done, as a convenience for the Action lambdas that are the actual commands executed, which do need access to the room, the world, and the GameResponse, which is held in the world.

The Imperative object carries the World and Room instances to the action lambdas, and because it is created early and dragged around for a while, those members are lateinit, that is, not provided on creation, but needing to be filled in before the object is complete. From a design viewpoint, this object changes over time. It is not cohesive. For much of its existence, it can’t possibly do its real job, passing information to the action lambdas.

There are a few levels at which we find ourselves testing the whole command cycle. We have tests for the Imperative, for the Actions object, for commands in rooms, and for commands in the world. Probably others. Setup for these is often complicated, because the tests have to set up and be concerned with objects that really don’t concern them, but that we need in order for the test to be able to get its result.

A number of objects are involved in the command cycle, notably including the Lexicon, which holds “synonyms” mapping words the user might type to a central “official” one that we use to define commands. The lexicon knows “verbs”, which provide a translation from a word like “east” to the standard command “go east”. Finally, the Lexicon holds “actions”, which performs a mapping from the verb and noun to a lambda, whose execution performs the actual command.

In particular, my current design thinking causes me to think that the Lexicon should not contain the actions, since we want to have actions defined at the world level, plus others defined only in a particular room.

There’s more to be aware of, but to give more detail than that, I’d really want to have the code in front of me, which is a pretty indication that the situation is more complicated than it might be.

Our mission this morning, and we choose to accept it, is to improve this situation. I do have some tentative ideas.

1. If the command method of Room were to return the response, test setup and execution might be made easier. The response should still be kept in World as well.
2. The command method does several things and will soon do more. It almost certainly should be broken up, and if it were, it might be easier to test.
3. The Imperative should probably come into being later, when all of its contents are available. It should probably be quite ephemeral, used only during lambda execution.
4. Phrase should perhaps become more prominent. I think the ImperativeFactory / Imperative should likely become a PhraseFactory producing a Phrase, while the Imperative gets created at the last possible moment.
5. The Imperative should probably contain a Phrase rather than the two strings verb and noun, though it might still offer those as properties to its lambdas.

I’m holding on to these ideas as lightly as I can, but I confess that I do feel pretty sure about them, because some or all of them are probably at least somewhat mistaken. I am an imperfect being, and I’m not the only imperfect being who is reading this paragraph. My design ideas are always imperfect, often even the ones I’m most sure about. It behooves me to hold them lightly, while always trying to build them well.

Let’s get to it. Let’s start with the command method in Room, which is one place where all this tangling affects us.

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

Right. I mentioned imperfect. I forgot to mention the Command, which, if I’m not mistaken, is just the String that the user typed in. Right:

data class Command(val input: String)

The command method here is, I think, even worse than it looks. Does it look “OK” to you? Let’s see what’s not to like:

1. There’s no way to test this method without passing it a world.
2. The second line tells us that the world has to have a valid Lexicon.
3. We get an Imperative back but while it may have the world plugged into it, it certainly doesn’t have the room plugged in. We do that near the end, where we create a new Imperative from the one we already have.
4. There are two lines here that are clearly just used in testing, so that some test can check the verb and noun that the Imperative found.
5. The Imperative act method accepts a lexicon. I think it only needs the actions and our new thinking requires us to be able to act on either the world’s actions or those of the room.
6. The act method does at least two things, it looks up the action and then executes it, passing the imperative to the action.

I think I could come up with as many things not to like as there are statements in this method! I’d only need one more! But these will do.

As always, the problem facing us is how to make this better. Fortunately, there is a pattern to follow, called Composed Method. I believe Kent Beck named this one. the pattern is, roughly:

When a method does more than one thing, create named methods for each things it does and replace the original method with a sequence of calls to those methods.

I’m tempted to deal with those stupid test result lines first. All that is happening there is that there’s a test somewhere that wants to know what the Imperative turned out to be. Let’s find that test and see what we can do to help it.

IDEA will help me, by giving me the lines that access those variables. There’s exactly one test:

    @Test
    fun roomsWithGo() {
        val world = world {
            room("woods") {
                go("s","clearing")
            }
            room("clearing") {
                go("n", "woods")
            }
        }
        assertThat(world.roomCount).isEqualTo(2)
        assert(world.hasRoomNamed("clearing"))
        val player = Player(world, "clearing")
        player.command("go n")
        assertThat(world.testVerb).isEqualTo("go")
        assertThat(world.testNoun).isEqualTo("n")
        assertThat(world.response.nextRoomName).isEqualTo("woods")
    }

It seems to me legitimate to drop those two assertions, since if the nextRoomName is “woods” the command was surely “go n”. I am tempted to force myself to come up with a better way to get those values to the test, just because the change might be educational, but I think the real case is that what that code is testing is whether “go n” is translated into verb=”go”, noun=”n”, and that’s not the job of this test.

I’m deleting those two assertions and the variables in world and setters in command.

Test. Green. Commit: Remove invasive test values from world and room.command, and the test that wanted them.

Review command again:

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

I am just about certain that my next move will be to extract those two lines creating the factory and using it. In fact, sure, let’s do that. I’ll figure out what keystrokes in IDEA will do it for me.

The command is option-command-M, which probably stands for Method, despite that it’s in the heading Function… in the control-T menu that you use to get available refactorings. Doing the command provides this:

    fun command(command: Command, world: World) {
        world.response.nextRoomName = roomName
        val imperative = makeImperative(world, command)
        val imp = Imperative(imperative.verb,imperative.noun,world, this)
        imp.act(world.lexicon)
    }

    private fun makeImperative(
        world: World,
        command: Command
    ): Imperative {
        val factory = ImperativeFactory(world.lexicon)
        val imperative = factory.fromString(command.input)
        return imperative
    }

That’s nearly good but I’d at least inline those last two lines. Command-option-N (for iNline) gives me this:

    private fun makeImperative(
        world: World,
        command: Command
    ): Imperative {
        val factory = ImperativeFactory(world.lexicon)
        return factory.fromString(command.input)
    }

I don’t need those parameters on separate lines. Will IDEA do that for me?

Yes, clicking on one of them gives me the yellow light, which offers “put parameters on one line”, resulting in this:

    private fun makeImperative(world: World, command: Command): Imperative {
        val factory = ImperativeFactory(world.lexicon)
        return factory.fromString(command.input)
    }

I could inline those two lines but in general that would not be the house style, so we’ll leave this … no, wait. Instead of passing in the world, whose only purpose is to provide the lexicon, let’s just pass the lexicon. That may turn out to be useful and in any case the method doesn’t need the whole world.

I’ll try the Change Signature here. That’s Command-F6. I don’t know why. The refactoring doesn’t want to do what I want. I’ll just type it, it’s right here in front of me.

    private fun makeImperative(command: Command, lexicon: Lexicon): Imperative {
        val factory = ImperativeFactory(lexicon)
        return factory.fromString(command.input)
    }

That triggers red in the calling method, which I fix. Let’s look at both of these together:

    fun command(command: Command, world: World) {
        world.response.nextRoomName = roomName
        val imperative = makeImperative(command, world.lexicon)
        val imp = Imperative(imperative.verb,imperative.noun,world, this)
        imp.act(world.lexicon)
    }

    private fun makeImperative(command: Command, lexicon: Lexicon): Imperative {
        val factory = ImperativeFactory(lexicon)
        return factory.fromString(command.input)
    }

Test all this. Green. Commit: Extract makeImperative from Room.command.

Let’s look further. One oddity in what we have here is that the imperative that comes back from makeImperative clearly doesn’t have the world or the room in it, and the imp that we create from it, clearly does.

So the ImperativeFactory isn’t really creating an Imperative. It creates half an imperative, the part with the verb and noun. Let’s take a look at it and see what really happens in ImperativeFactory.fromString.

class ImperativeFactory(private val lexicon: Lexicon) {

    fun fromOneWord(verb:String): Imperative = imperative(verb)
    fun fromTwoWords(verb:String, noun:String): Imperative
        = imperative(verb).setNoun(synonym(noun))
    private fun imperative(verb: String) = lexicon.translate(synonym(verb))
    private fun synonym(verb: String) = lexicon.synonym(verb)

    fun fromString(input: String): Imperative {
        val words = input.lowercase().split(" ")
        return when (words.size) {
            1-> fromOneWord(words[0])
            2-> fromTwoWords(words[0], words[1])
            else -> fromTwoWords(":tooManyWords", input)
        }
    }
}

Looks like we’d better look at Lexicon as well.

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): Imperative = verbs.translate(word)
    fun act(imperative: Imperative) = actions.act(imperative)
}

class Synonyms(private val map: Map<String,String>) {
    fun synonym(word:String) = map.getValue(word)
}

class Verbs(private val map:Map<String, Imperative>) {
    fun translate(verb:String): Imperative = map.getValue(verb)
}

We have a perfectly good object that contains the verb and noun, the Phrase. It seems to me that if the ImperativeFactory returned a Phrase rather than an Imperative, the oddity of the half-built Imperative would be resolved.

It looks like a fair number of things might need to change, including the lexicon’s Verbs, which are a map from verb to Imperative.

Let’s go in stages. Let’s say that the ImperativeFactory is going to return a Phrase rather than an Imperative, but do the translation internally. The only place we get an Imperative in here is in the method translate in Verbs.

I want to try something, to see how big a change this might be. I’m considering two options. One is to change the final return of the Factory to be a Phrase. Another would be to work to get Verbs to produce a Phrase. Let’s do the first, because it removes a bit of ugliness at the higher level. I’m just going to cast the final Imperative to a Phrase and see what breaks besides command. I’ll start with command, changing its expectations, changing this:

    fun command(command: Command, world: World) {
        world.response.nextRoomName = roomName
        val imperative = makeImperative(command, world.lexicon)
        val imp = Imperative(imperative.verb,imperative.noun,world, this)
        imp.act(world.lexicon)
    }

To this:

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

    private fun makeImperative(command: Command, lexicon: Lexicon): Phrase {
        val factory = ImperativeFactory(lexicon)
        val imp = factory.fromString(command.input)
        return Phrase(imp.verb, imp.noun)
    }

I had to add the bangs to fetching the verb and noun, because Phrase allows nulls for those, which is part of its design, and probably not a truly great part. One concern at a time, however.

We could perhaps do better by asking the Phrase to give us the Imperative. Even better, we should probably change the Imperative to have a Phrase, not the separate verb and noun. Or, possibly, we could get rid of the imperative entirely, but I think that’s a bit too far to leap.

Anyway tests are green. Commit: Room.command now expects a Phrase back from makeImperative. No, wait, don’t commit, first rename that to makePhrase. Now commit: Room.command expects a phrase back from makePhrase, formerly half-baked Imperative.

Review the code again:

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

    private fun makePhrase(command: Command, lexicon: Lexicon): Phrase {
        val factory = ImperativeFactory(lexicon)
        val imp = factory.fromString(command.input)
        return Phrase(imp.verb, imp.noun)
    }

Let’s look at act on the Imperative. I suspect it doesn’t want the whole lexicon, and I know we don’t want to give it the whole lexicon. We will also see whether we can just pass in the Phrase.

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

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

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

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

Well isn’t that nice? We really only use a phrase made from the verb and noun. So let’s see about changing the Imperative to have a Phrase rather than verb and noun.

    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(world.lexicon)
    }

The compiler isn’t happy with that, of course. The Imperative is a bit tricky, and extensively tested. But let’s just see what happens if we change this:

data class Imperative(
    val verb: String,
    val noun: String,
    val world: World,
    val room: Room
) 

To this:

data class Imperative(
    val phrase: Phrase,
    val world: World,
    val room: Room
) 

IDEA/Kotlin want me to change this:

    fun setNoun(noun: String): Imperative {
        return Imperative(verb, noun, world, room)
    }

I think that’s just this:

    fun setNoun(noun: String): Imperative {
        return Imperative(Phrase(phrase.verb, noun), world, room)
    }

So far no one else is complaining but I’m sure running the tests is going to find people asking for verb or noun. Let’s find out.

First issue, hoist by my own petard:

    private fun makePhrase(command: Command, lexicon: Lexicon): Phrase {
        val factory = ImperativeFactory(lexicon)
        val imp = factory.fromString(command.input)
        return Phrase(imp.verb, imp.noun)
    }

I should be able to return the Imperative’s phrase here, I think.

    private fun makePhrase(command: Command, lexicon: Lexicon): Phrase {
        val factory = ImperativeFactory(lexicon)
        val imp = factory.fromString(command.input)
        return imp.phrase
    }

But there are innumerable other references. Let’s look at a few. We can always provide accessors in Imperative if we need them.

A lot of them are in the commands, which like to check the verb and noun. Let’s do provide accessors in Imperative.

Ah. A few quick tries and I find that the maps for Verbs are creating imperative instances, and they all dislike that change to Phrase. Let’s check it out but we may want to roll back.

    private fun makeVerbs(): Verbs {
        return Verbs(mutableMapOf(
            "go" to Imperative("go", "irrelevant", this, Room("fred")),
            "e" to Imperative("go", "e", this, Room("fred")),
            ...
        ).withDefault { (Imperative(it, "none", this, Room("fred")))})
    }

Well, there are a lot of them, but let me just try to mass edit them and see what happens next. I still favor a rollback unless this sorts most everything.

One multi-cursor edit later:

    private fun makeVerbs(): Verbs {
        return Verbs(mutableMapOf(
            "go" to Imperative(Phrase("go", "irrelevant"), this, Room("fred")),
            "e" to Imperative(Phrase("go", "e"), this, Room("fred")),
            "w" to Imperative(Phrase("go", "w"), this, Room("fred")),
            "n" to Imperative(Phrase("go", "n"), this, Room("fred")),
            "s" to Imperative(Phrase("go", "s"), this, Room("fred")),
            "say" to Imperative(Phrase("say", "irrelevant"), this, Room("fred")),
            "xyzzy" to Imperative(Phrase("say", "xyzzy"), this, Room("fred")),
            "wd40" to Imperative(Phrase("say","wd40"), this, Room("fred")),
        ).withDefault { (Imperative(Phrase(it, "none", this), Room("fred")))})
    }

There’s another to be done, I’m sure. Oh, and that last line didn’t edit well, should be:

).withDefault { (Imperative(Phrase(it, "none"), this, Room("fred")))})

Further changes:

    @Test
    fun `create Imperative`() {
        val imp = Imperative(Phrase("go", "east"), world, room)
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
    }

        imp = Imperative(Phrase("forge", "sword"), world, room)

And so on. I’ll spare you the details. Let me finish these up and see what the tests then say.

The tests say Green. Commit: Converted Imperative to have a Phrase, not separate verb and noun. Those are provided by accessors.

OK, time for a time check and reflection.

Time is 0734, so I’ve been at this around 2 1/2 hours, mostly writing. (I think about 2/3 of my time when I do this goes to writing the article. might be fun to have a chess clock or something, to find out exactly. There are some long pauses for thinking sometimes. We might split those 50-50 between writing thinking and programming thinking.) Some time was spent feeding the cat and making my iced Chai, but probably no more than 6 minutes.

Reflection

Here I want to talk about what we’ve accomplished more than any particular observations about how it came about. Everything went very directly. There were perhaps as many as 30 lines changed in that last commit, but they were all the same, just grabbing verb and noun inside Phrase(). The other changes were small, and IDEA clicked me right through them. I’m starting to like this tool set, as I’ve mentioned before.

Let’s look at command again:

    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(world.lexicon)
    }

    private fun makePhrase(command: Command, lexicon: Lexicon): Phrase {
        val factory = ImperativeFactory(lexicon)
        val imp = factory.fromString(command.input)
        return imp.phrase
    }

There’s an oddity here in this code. I’m aware of what’s going on and why but even if we weren’t, it should raise an eyebrow. The makePhrase returns a Phrase, which is immediately turned into an Imperative. But makePhrase itself actually had an Imperative. Why didn’t we just return it?

There are two reasons, neither of them entirely obvious.

First, the Imperative we get back from fromString is incomplete. It is malformed. It is bad. It has no world and no room component.

Second, we are in the middle of a mission, which is to convert the Imperative factory into a Factory producing Phrases. Or, that’s what we (OK, I) had in mind when I made the change to return a Phrase. Instead of building a broken Imperative, we could build a perfectly good Phrase instead.

So our next step, from that point of view, might be to change fromString to return a Phrase, and see what happens. I’m not sure whether a lot of tests rely on that or not. If they do, they’re probably only referring to verb and noun anyway.

I’ll get the senders of fromString, since now I’ve made myself interested.

WARNING

Everything from here down to After Rollback gets rolled back. Skim or skip forward as you wish.

There are six tests, and the line above. Just seven calls. Let’s examine the tests:

    fun `raw string input`() {
        val factory  = getFactory()
        var imp = factory.fromString("east")
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
        ...

All six of the calls seem to be in that one test. Easy-peasy.

Push returning the phrase down. Will IDEA help me with that signature change? Worth a try.

Well, the good news is it lets me change the signature, but the bad news is it leaves all the work to me. Oh well, I’m used to it.

I trek through changing things. This was key:

class Verbs(private val map:Map<String, Imperative>) {
    fun translate(verb:String): Phrase {
        val uh  = map.getValue(verb)
        return uh.phrase
    }
}

I named that uh because I didn’t know for sure what came back. It was an imperative, so I just fetched its phrase. Remember, we think we want to change the verb maps to return phrases directly, but that’s not done.

That’s the last thing I did in one go. The tests will show me what’s left.

class ImperativeFactory(private val lexicon: Lexicon) {

    fun fromOneWord(verb:String): Phrase = imperative(verb)
    fun fromTwoWords(verb:String, noun:String): Phrase
        = imperative(verb).setNoun(synonym(noun))

The compiler doesn’t like the fact that phrase doesn’t know setNoun. I don’t like the fact that I changed the imperative method to return a Phrase but didn’t rename the method. In Phrase:

    fun setNoun(noun: String): Phrase {
        return Phrase(this.verb,noun)
    }

We return a new one because we want things to be immutable. And in this code:

class ImperativeFactory(private val lexicon: Lexicon) {

    fun fromOneWord(verb:String): Phrase = getPhrase(verb)
    fun fromTwoWords(verb:String, noun:String): Phrase
        = getPhrase(verb).setNoun(synonym(noun))
    private fun getPhrase(verb: String): Phrase = lexicon.translate(synonym(verb))
    private fun synonym(verb: String) = lexicon.synonym(verb)

    fun fromString(input: String): Phrase {
        val words = input.lowercase().split(" ")
        return when (words.size) {
            1-> fromOneWord(words[0])
            2-> fromTwoWords(words[0], words[1])
            else -> fromTwoWords(":tooManyWords", input)
        }
    }
}

Test. This is no longer valid: it’s what we set out to change:

    private fun makePhrase(command: Command, lexicon: Lexicon): Phrase {
        val factory = ImperativeFactory(lexicon)
        val imp = factory.fromString(command.input)
        return imp.phrase
    }

And the fix is:

    private fun makePhrase(command: Command, lexicon: Lexicon): Phrase {
        val factory = ImperativeFactory(lexicon)
        return factory.fromString(command.input)
    }

Test. Kind of thinking green would be nice. Not yet, some tests need modification. Here’s one:

    @Test
    fun `look up some imperatives`() {
        val imperatives = getFactory()
        var imp: Imperative = imperatives.fromOneWord("east")
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
        imp = imperatives.fromOneWord("e")
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
    }

Right, well, we know that’s returning phrases now. Rename and recast:

    @Test
    fun `look up some imperatives`() {
        val imperatives = getFactory()
        var phrase: Phrase = imperatives.fromOneWord("east")
        assertThat(phrase.verb).isEqualTo("go")
        assertThat(phrase.noun).isEqualTo("east")
        phrase = imperatives.fromOneWord("e")
        assertThat(phrase.verb).isEqualTo("go")
        assertThat(phrase.noun).isEqualTo("east")
    }

I expect that to run. Yes. The other tests are a bit more tricky to fix, they’re asking the result of fromString to do things:

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

I can think of a couple of ways to go here. One is just to create the expected Imperative. We can see that it’s malformed, since it won’t have a world or room, but the test ones don’t refer to those members. Another possibility might be to gin up something to make the Phrase executable. I think that’s off, unless and until we change the current focus of Actions on the Imperative.

What are we testing here? It appears that we’re really testing the lookup and call of the imperative.

I’ll try creating one, in the test above. I think I would like to say this:

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

However, the Imperative doesn’t have a prefix-only constructor. As a data class, can it have optional parameters?

data class Imperative(
    val phrase: Phrase,
    val world: World?=null,
    val room: Room?=null
) 

I’ll try it. I’m required to change this to a safe call:

    fun say(s: String) = world?.say(s)

Test, but with big concerns. No, that change is going to propagate mull checks all over. I’m in too deep. I could try backing that last change out, but I think a rollback is called for.

I think I’ve lost more changes than I’d like to have. I don’t remember whether I had a save point in between all the stuff above or not. Time to refresh my mind on where we are.

After Rollback

    private fun makePhrase(command: Command, lexicon: Lexicon): Phrase {
        val factory = ImperativeFactory(lexicon)
        val imp = factory.fromString(command.input)
        return imp.phrase
    }

Ah yes, we wanted to change the factory to return a Phrase, not an imperative. But we have some tests that are counting on it to return an imperative.

There’s an issue here, which is that those tests are relying on a quirk in the Verbs setup. The Verbs table maps a verb (String) to an Imperative. In the test verbs, the world and room are set up locally. In the “real” world, the Verbs are only created after the world is created. They are literally hacked to contain a room:

    private fun makeVerbs(): Verbs {
        return Verbs(mutableMapOf(
            "go" to Imperative(Phrase("go", "irrelevant"), this, Room("fred")),
            "e" to Imperative(Phrase("go", "e"), this, Room("fred")),
            "w" to Imperative(Phrase("go", "w"), this, Room("fred")),
            "n" to Imperative(Phrase("go", "n"), this, Room("fred")),
            "s" to Imperative(Phrase("go", "s"), this, Room("fred")),
            "say" to Imperative(Phrase("say", "irrelevant"), this, Room("fred")),
            "xyzzy" to Imperative(Phrase("say", "xyzzy"), this, Room("fred")),
            "wd40" to Imperative(Phrase("say","wd40"), this, Room("fred")),
        ).withDefault { (Imperative(Phrase(it, "none"), this, Room("fred")))})
    }

In actual use, we create a new Imperative at the last moment, so that both the world, which is the same, and the room, which isn’t, are overridden with the correct values at runtime.

Let’s see if I can default the world and room in Imperative. Nulling them didn’t work: this might.

data class Imperative(
    val phrase: Phrase,
    val world: World = world(){},
    val room: Room = Room("fakeroom")
) 

This doesn’t bother the compiler. Let’s see if it bothers the tests. Shouldn’t: I don’t think anyone is taking advantage of this new “feature” yet. Right, still green.

Now let’s change fromString to return a Phrase. That will require me to deal with all seven senders. We start with this:

    fun fromString(input: String): Imperative {
        val words = input.lowercase().split(" ")
        return when (words.size) {
            1-> fromOneWord(words[0])
            2-> fromTwoWords(words[0], words[1])
            else -> fromTwoWords(":tooManyWords", input)
        }
    }

We want it to return a Phrase, and we want to do it in tiny steps. The three cases of the when currently return an Imperative. We need to change all of them but let’s first get the Phrase returned and fix the tests.

    fun fromString(input: String): Phrase {
        val words = input.lowercase().split(" ")
        val imp = when (words.size) {
            1-> fromOneWord(words[0])
            2-> fromTwoWords(words[0], words[1])
            else -> fromTwoWords(":tooManyWords", input)
        }
        return imp.phrase
    }

This is an example of taking smaller steps. Last time, I think I fiddled around with all the internal methods before I had a stable place to stand. This time I can fix the senders and … with luck and what skill I have … get to green soon.

This won’t compile, of course:

    private fun makePhrase(command: Command, lexicon: Lexicon): Phrase {
        val factory = ImperativeFactory(lexicon)
        val imp = factory.fromString(command.input)
        return imp.phrase
    }

This is straightforward. Already did this once.

    private fun makePhrase(command: Command, lexicon: Lexicon): Phrase {
        val factory = ImperativeFactory(lexicon)
        return factory.fromString(command.input)
    }

The tests will complain. I think all the issues are in one test, if I recall.

Wait, what? The tests are all running. How’s that happening? I was sure there were users of fromString that can’t cope. Look for them.

The test I had in mind was this one:

    @Test
    fun `raw string input`() {
        val factory  = getFactory()
        var phrase = factory.fromString("east")
        assertThat(phrase.verb).isEqualTo("go")
        assertThat(phrase.noun).isEqualTo("east")
        phrase = factory.fromString("go west")
        assertThat(phrase.verb).isEqualTo("go")
        assertThat(phrase.noun).isEqualTo("west")
        phrase = factory.fromString("Go West")
        assertThat(phrase.verb).isEqualTo("go")
        assertThat(phrase.noun).isEqualTo("west")
        val tooMany = "too many words"
        phrase = factory.fromString(tooMany)
        assertThat(phrase.verb).isEqualTo(":tooManyWords")
        assertThat(phrase.noun).isEqualTo(tooMany)
        phrase = factory.fromString("")
        assertThat(phrase.verb).isEqualTo("")
        assertThat(phrase.noun).isEqualTo("none")
        val noWords = "234563^%$#@"
        phrase = factory.fromString(noWords)
        assertThat(phrase.verb).isEqualTo(noWords)
        assertThat(phrase.noun).isEqualTo("none")
    }

It was OK except it said imp instead of phrase. The problems causing my rolback must have arisen later. I should have made this a save point. I will, this time. Commit: save point, ImperativeFactory fromString returns a Phrase.

Seems likely that we should rename ImperativeFactory to PhraseFactory, doesn’t it? Let’s review it.

class ImperativeFactory(private val lexicon: Lexicon) {

    fun fromOneWord(verb:String): Imperative = imperative(verb)
    fun fromTwoWords(verb:String, noun:String): Imperative
        = imperative(verb).setNoun(synonym(noun))
    private fun imperative(verb: String) = lexicon.translate(synonym(verb))
    private fun synonym(verb: String) = lexicon.synonym(verb)

    fun fromString(input: String): Phrase {
        val words = input.lowercase().split(" ")
        val imp = when (words.size) {
            1-> fromOneWord(words[0])
            2-> fromTwoWords(words[0], words[1])
            else -> fromTwoWords(":tooManyWords", input)
        }
        return imp.phrase
    }
}

This is still working with Imperatives, and if those top methods are public and in use, there are people out there who know what we’re doing. Let’s check senders of each.

There are 8 tests looking at fromOneWord. Here’s a typical one:

    @Test
    fun `look up some imperatives`() {
        val imperatives = getFactory()
        var imp: Imperative = imperatives.fromOneWord("east")
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
        imp = imperatives.fromOneWord("e")
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
    }

We’d better check the others, this seems like where I got in trouble before.

Ah, yes, it was the ones like this, that test actions:

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

I’m going to do something in this test. I think this is either a decent creative idea or truly bad. I’m going to grab the phrase out of the imperative above, and then create a new imperative from it. If that works, then causing that method to return a phrase will also work, with a small change that we’ll see.

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

Test just this one. It runs. I think this trick will work throughout. Change fromOneWord to return a Phrase.

    fun fromOneWord(verb:String): Phrase = imperative(verb).phrase

I should perhaps have started with fromTwoWords. I think these methods call each other so we’ll perhaps need to do more work than is desirable. Let’s test and see.

Right. We’ll have to resolve all three of the branches of the when at once. It looks like this:

    fun fromString(input: String): Phrase {
        val words = input.lowercase().split(" ")
        val imp = when (words.size) {
            1-> fromOneWord(words[0])
            2-> fromTwoWords(words[0], words[1])
            else -> fromTwoWords(":tooManyWords", input)
        }
        return imp.phrase
    }

So we have to fix up two words also. That demands setNoun on phrase:

    fun fromTwoWords(verb:String, noun:String): Phrase
        = imperative(verb).setNoun(synonym(noun))

That, at least, we can safely do.

    fun setNoun(noun: String): Phrase {
        return Phrase(this.verb,noun)
    }

This seems familiar. If we roll back again, remind me to commit that bit.

Now the factory looks like this:

class ImperativeFactory(private val lexicon: Lexicon) {

    fun fromOneWord(verb:String): Phrase = getVerbPhrase(verb)
    fun fromTwoWords(verb:String, noun:String): Phrase
        = getVerbPhrase(verb).setNoun(synonym(noun))
    private fun getVerbPhrase(verb: String): Phrase = lexicon.translate(synonym(verb)).phrase
    private fun synonym(verb: String) = lexicon.synonym(verb)

    fun fromString(input: String): Phrase {
        val words = input.lowercase().split(" ")
        val phrase = when (words.size) {
            1-> fromOneWord(words[0])
            2-> fromTwoWords(words[0], words[1])
            else -> fromTwoWords(":tooManyWords", input)
        }
        return phrase
    }
}

I think I’m ready to test and fix up the fails. Unless something surprising happens. The first one is easy:

    @Test
    fun `look up some imperatives`() {
        val imperatives = getFactory()
        var phrase: Phrase = imperatives.fromOneWord("east")
        assertThat(phrase.verb).isEqualTo("go")
        assertThat(phrase.noun).isEqualTo("east")
        phrase = imperatives.fromOneWord("e")
        assertThat(phrase.verb).isEqualTo("go")
        assertThat(phrase.noun).isEqualTo("east")
    }

Just had to change the expectation and name to phrase.

Then there’s the one where I tested the idea:

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

Now the fromOneWord is returning a Phrase. So … that thing is calling loudly for a method on Phrase to create an Imperative. We’ll want it to accept world and room in due time but for now let’s just do this:

class Phrase ...
    fun asImperative() = Imperative(this)

And in our tests, as a large example:

    @Test
    fun `one and two word commands`() {
        val factory  = getFactory()
        var imp = factory.fromTwoWords("go", "w").asImperative()
        assertThat(imp.act(testLex())).isEqualTo("went west")
        imp = factory.fromOneWord("w").asImperative()
        assertThat(imp.act(testLex())).isEqualTo("went west")
        imp = factory.fromOneWord("xyzzy").asImperative()
        assertThat(imp.act(testLex())).isEqualTo("said xyzzy")
        imp = factory.fromTwoWords("say", "xyzzy").asImperative()
        assertThat(imp.act(testLex())).isEqualTo("said xyzzy")
        imp = factory.fromTwoWords("say", "unknownWord").asImperative()
        assertThat(imp.act(testLex())).isEqualTo("said unknownWord")
        imp = factory.fromTwoWords("unknownVerb", "unknownNoun").asImperative()
        assertThat(imp.act(testLex())).isEqualTo("I can't unknownVerb a unknownNoun")
        imp = factory.fromOneWord("unknownWord").asImperative()
        assertThat(imp.act(testLex())).isEqualTo("I can't unknownWord a none")
        imp = factory.fromOneWord("inventory").asImperative()
        assertThat(imp.act(testLex())).isEqualTo("You got nothing")
    }

I’m totally green. Commit: ImperativeFactory is making Phrases, not Imperatives.

I did that because I wanted to get those hanging changes into the Repo as soon as I could. Now, of course, we should rename the factory. Idea does that nicely for us. Commit: Rename ImperativeFactory to PhraseFactory.

It’s 0932. Been here since 0508, 4 1/2 hours. This is getting to be like a job, except that I’m having fun and not getting paid.

I have one major reflection to offer before I even decide what to do next.

Reflection (Major)

The first pass through converting to using Phrases resulted in a rollback. I got too deeply in and my little house of cards collapsed. That’s a sure sign that my steps are too big: with small steps from green to green, even I can manage one card flat on the table. It’s when I start needing a longer series of moves to work that I get in trouble. I think that is natural and inevitable.

Suppose that in any given change, I have a probability of a mistake of some small number, like, oh, how about 1/4. Sure. I’ll accept that: in one out of four changes, I might make a mistake. You, of course, are closer to one in twenty or one hundred or something. Sure. Go with me here.

So I’m correct 3/4 of the time when I make one change. If I make two changes, my chances of having made an error are 43%. Three changes? 68%. Ten? 94% of the time, I’ll have a mistake in there somewhere.

You, with your one in twenty? You’ve got about a 40 percent chance of an error in ten changes. One in a hundred, you say? You still have a ten percent chance of an error in ten changes.

And the thing is, after ten changes, when an error shows up, we’ve got a debugging situation on our hands. When after one change, an error shows, we either see it and fix it, or back it out and do it again. Either way, very rarely do we need to debug.

That’s why small steps, trivially small steps, are faster than larger ones. Fewer surprises, less debugging, and way less going WTF.

So small steps work, not because we’re smart, not only if we happen to be particularly dull, but because when we chain probabilities, the bad news comes really soon. That’s why a trifecta pays off so much: calling three horses is way ore than three times harder than calling one.

Wow. I just realized that this article is over 1000 lines long. that’s a lot. Let’s wrap up.

Summary

Eight commits. Not bad, given the diversion where I had to roll back. And, again, I don’t consider those to be a failure just a learning experiment. Only if I found myself giving up on the whole idea would I feel a sense of failure, and even then, I feel sure I’ll have a better idea tomorrow. In a way, it’s like writing. We write a draft, revise, sometimes throw away and rewrite. Unless you’re one of those stream of consciousness writers like YT in these articles. But that’s intentional: I mostly want you to see what I think and what happens. And, I do revise: I’m revising this paragraph right now.

We’ve improved things, and I think the improvement is more valuable than it may appear at first glance. We’ve got a much simpler object, the Phrase, sliding through the command processing, right up until the last minute, where we plug in the world and room and turn an Action loose to do the work.

Much simpler, and we’re not done yet. I think we’d like to change the Verbs map to return a Phrase instead of an Imperative. That won’t take long at all, even though there are quite a few lines involved. We already changed all the Imperatives to contain a Phrase, and that included changing all the verb maps. So we can be sure that the other change, with the help of IDEA, will likely also be easy.

A good morning, even if it did start at Sparrow’s.

Join me next time, please! And, if you’re out there, please let me know via a tweet or a like. I like to know there’s a listener out there somewhere.