GitHub Repo

There’s not really much left to do here, if my purpose is to build a DSL for Zork-like games. That’s a big problem. Today: A nice refactoring in tiny steps.

My real purpose for this program, of course, was to get some experience with Kotlin, because my Friday Geek’s Night Out group was talking about jointly writing something, and we thought Kotlin was an interesting choice, and so I needed to build up some chops in IDEA and Kotlin. More or less at random, because of something I saw, I wanted to start with the Kotlin style DSL builders, and a game like Adventure or Zork came to mind. And now here we are.

I think the code now has essentially all it needs to be able to build Adventure. If Zork used more than two words per command, we’re not there for Zork, but I don’t even know if that’s interesting. So, let’s say it’s essentially there. I could build out a game, either an Adventure clone or a game of my own invention. There’d be some programming learning, and probably some testing learning, since the code for the game would be almost entirely in lambdas, so I’d need to devise ways to test them.

But the game … no one could play the game, even if they wanted to, unless.

  1. They cloned the project and built it just to play the game.
  2. I created and released an executable.
  3. I created an on-line version.

None of those seems very likely to be fun. So, very soon, I’m going to need a new project. Now the FGNO group might decide to carry through on the idea of a joint project, but I’m not optimistic. We seem to me to have wandered back into our usual paths, maintaining our evening meetings once a week, but working on our own concerns. We’ll see.

So I may have a big problem, which will come in at least two parts:

  1. Come up with a new project;

  2. Get the environment set up to do it.

Fortunately … there’s a bit left to do in the ADVENt program, and I can probably entertain myself building the game for a while.

Things We Might Do …

  1. The World’s Actions are inside the World’s Lexicon. The Room’s are in the room. This isn’t symmetric and they should move from Lexicon to World. This is probably almost trivially easy.

  2. The Verbs map is from verb to Imperative. This is just wrong, as we now create a new Imperative at the last minute. This, too, should be easy to clean up.

  3. The handling of the GameResponse is weird. It “should” be created right at the time of executing a command, but for reasons of access, it’s created long before then and passed around via World. That should be sorted. Again, probably not difficult.

  4. I would like to improve the output window a bit but this is purely cosmetic and it doesn’t need much, just to be larger with a larger typeface. Boring.

  5. Track down all the IDEA/Kotlin warnings and fix them, learning a bit about Kotlin in the process. Meh.

So I need ideas, folks, I need ideas. And no, I don’t want to undertake Go or Closure or another new language just now. I’d like to stay in here a while.

Meanwhile …

Let’s look at Verbs and see if we can’t simplify what’s going on there.

The table construction itself looks like this:

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

But how are they used? That must be in the PhraseFactory. (Remind me that I think it should be renamed to PhraseMaker. It just sounds better.)

class PhraseFactory(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
    }
}

We’re making a phrase. The access to the verbs is via lexicon.translate. Doubtless that returns an Imperative, and we rip the phrase out of it. Let’s verify:

Command-B on translate and Voila! we’re here:

class Lexicon ...
    fun translate(word: String): Imperative = verbs.translate(word)

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

So we can unwind this rather quickly We’ll need to modify all along that call chain. I don’t know whether IDEA has anything that clever, but let's just do it, starting at the bottom. There are probably at least two creators of tables for the verbs, but the compiler will find them. In Verbs, we want the map to be from String to Phrase:

No. As soon as I look at this, I see what should be a better way to go. Let’s see how many commits of green code we can get here. The idea is this: in real efforts, we often encounter a need for a “big change”, like changing the form of some key table, like Verbs. We would like to do this incrementally, over a number of days or even longer, so that we don’t have to create a long-lived branch in the code, or a long delay in delivering value to our users.

I think there is just the one call to translate on Lexicon. I’m mistaken. There are three:

    private fun getVerbPhrase(verb: String): Phrase = lexicon.translate(synonym(verb)).phrase

// some test ...
        val imp: Imperative = lexicon.translate(
            lexicon.synonym("e")
        )
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
        assertThat(imp.actForTesting(lexicon)).isEqualTo("went east")

// another test ...
        val imp = lex.translate("e")
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("e")

The first really wants a phrase. The third would work just as well if a phrase came back. The second really thinks it wants an Imperative. The test is in an environment where there is a world and a room available, because they’re needed in the verb setup. So we can change that second test thus:

    @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")
        ).phrase
        val imp = Imperative(phrase, world, room)
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
        assertThat(imp.actForTesting(lexicon)).isEqualTo("went east")
    }

This runs green. Commit: Revise test to use phrase to create imperative.

Now let’s do this:

data class Phrase(val verb: String?=null, val noun: String?=null) {
    val phrase = this

When sent the message phrase, a Phrase returns itself. No one is doing this, I didn’t write a test for it. Let me correct that oversight.

        val phrase: Phrase = lexicon.translate(
            lexicon.synonym("e")
        ).phrase
        assertThat(phrase.phrase).isEqualTo(phrase)

Test. Green. Commit: Phrase.phrase returns this. Preparation for migration of Verbs.

Now let’s see whether we can change the return of the translate to be a phrase. This may work nicely or may fail, depending on our tests. If a bunch break, we’ll go another way.

class Lexicon ...
    fun translate(word: String): Phrase = verbs.translate(word).phrase

Test, curious as to what will happen. Everything is green. This is big. Commit: Lexicon.translate returns a phrase, not an imperative.

Now we know that all the calls to Lexicon.translate are happy with a phrase coming back. That might not have been the case, but it was. Now what about calls to Verbs.translate?

There are only two. The one above, where we’re now returning a Phrase, and one test:

    @Test
    fun verbTranslator() {
        val verbs = getVerbs()
        val imp = verbs.translate("east")
        assertThat(imp.verb).isEqualTo("go")
        assertThat(imp.noun).isEqualTo("east")
    }

This guy would be happy with a phrase if he had one. So:

    @Test
    fun verbTranslator() {
        val verbs = getVerbs()
        val phrase = verbs.translate("east").phrase
        assertThat(phrase.verb).isEqualTo("go")
        assertThat(phrase.noun).isEqualTo("east")
    }

He’s green. Commit: Modified test to use phrase rather than imperative. Part of Verb refactoring.

Now we change change Verbs.translate to return a Phrase:

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

Test. Green. Commit: Verbs.translate returns a Phrase, not Imperative.

Now we can change the maps in Verbs. I believe there are only two, so we can just change them and be done with it. However, what if there were more?

The Verbs object is now returning a phrase unconditionally. This means we can remove our calls to phrase on Imperative, I think. IDEA will tell us.

The only references to Imperative.phrase are the one in Verbs, and some inside Imperatives itself. Other uses of the method are now referring to the method on Phrase. So we can remove this one:

    private fun getVerbPhrase(verb: String): Phrase = lexicon.translate(synonym(verb)).phrase

And this one:

    fun translate(word: String): Phrase = verbs.translate(word).phrase

We could have committed between those two. Commit now: Remove redundant calls to phrase, part of Verbs refactoring no longer needed.

Now Verbs.translate is the only point where the conversion is made, and no one is expecting an imperative back from that method: it returns Phrase:

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

Now we can fix the tables. There are just the two:

This will require a few changes at once. I think we could avoid that by creating an interface and changing the map to expect the interface, which we’d make consistent with both Phrase and Imperative, which would let me create the tables either way.

That’s not needed here, and I’m not inclined to do it just for purposes of showing that we could. We’ve already drilled this down to two tables. We’ll fix the signature here:

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

Now the two tables. I’ll let the compiler find them. Here’s one:

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

Looks like a job for multi-cursor editing to me.

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

Easy. Now the other, which the compiler will find:

private val TestVerbTable = mutableMapOf(
    "go" to Imperative(Phrase("go", "irrelevant"), world, room),
    "east" to Imperative(Phrase("go", "east"), world, room),
    "west" to Imperative(Phrase("go", "west"), world, room),
    "north" to Imperative(Phrase("go", "north"), world, room),
    "south" to Imperative(Phrase("go", "south"), world, room),
    "say" to Imperative(Phrase("say", "irrelevant"), world, room),
    "xyzzy" to Imperative(Phrase("say", "xyzzy"), world, room),
    ).withDefault { (Imperative(Phrase(it, "none"), world, room))
}

That becomes:

private val TestVerbTable = mutableMapOf(
    "go" to Phrase("go", "irrelevant"),
    "east" to Phrase("go", "east"),
    "west" to Phrase("go", "west"),
    "north" to Phrase("go", "north"),
    "south" to Phrase("go", "south"),
    "say" to Phrase("say", "irrelevant"),
    "xyzzy" to Phrase("say", "xyzzy"),
    ).withDefault { Phrase(it, "none") }

And we are green. Commit: Verbs map now from string to Phrase throughout. “Big” Refactoring complete.

So that was nice. It was done in six commits. Here’s how many lines were changed in each one:

  1. 5
  2. 1
  3. 3 (two were a rename of a variable)
  4. 1
  5. 2
  6. 19 (two tables of 9 and 8)

Now I wouldn’t go so far as to say that every refactoring can be done in steps of mostly 1 or two lines, but I do believe that we can (almost?) always find a way to break down what seems like a big change into lots of very small safe steps.

I think that “almost” is so close to “always” that we should always assume there is an incremental path and work a bit to find it, because small steps are safe and big refactorings definitely are not.

I actually believe that I might have done this better with a little more care, though looking at those figures I find it hard to believe. But I think that the trick of starting at the top of the tree and putting in the phrase call was a good move.

Oh! Just realized I could remove some of the extra calls to .phrase. One more commit, removed two lines, changed two.

The nice thing about that .phrase idea was that by making .phrase idempotent in Phrase, we could safely leave the transition calls in forever. Best not to, of course: their whole point was to be transitory. Perhaps I should have given them a more obnoxious name.

So, a nice refactoring changing out an entire data structure, done in separate safe commits of a few lines each. Lovely.

Come see me next time, please!