Refactoring in the direction of what we think we might prefer. Beck said: Make the change easy, then make the easy change.

I’m going to begin to improve my command parsing function, to be more ready for the new parsing when I plug that in over the next day or so, and I’m going to try to do it in tiny tiny steps. We’ll see how that goes.

Here’s my current command method in Room:

    fun command(cmd: String, world: World) {
        val name = when(cmd) {
            "take axe" -> take("axe", world)
            "take bottle" -> take("bottle", world)
            "take cows" -> take("cows", world)
            "inventory" -> inventory(world)
            "s","e","w","n" -> move(cmd, world)
            "xyzzy" -> move("xyzzy", world)
            "cast wd40" -> castSpell("wd40", world)
            else -> "unknown cmd $cmd"
        }
        world.response.nextRoomName = name
    }

There are a number of things that I don’t like about this, and I’ve been experimenting in the two preceding articles with improvements. One thing that I don’t like is the need to remember to pass “world” to all the calls. I propose to fix that by returning the function that needs to be called, and calling it once only.

To begin with, we will need to get all these functions to accept the same calling sequence. Looking at them, I think we need three parameters on each, which I’ll call verb, noun, and world. Most of them will ignore verb, and move will ignore noun.

Let’s change the signatures, one at a time. We’ll see whether IDEA will help us with this, or just get in the way. I’ll start with take:

    private fun take(item:String, world: World): String {
        world.take(item)
        world.response.say("$item taken.")
        return roomName
    }

First I’ll rename item to noun:

    private fun take(noun:String, world: World): String {
        world.take(noun)
        world.response.say("$noun taken.")
        return roomName
    }

As this is a machine refactoring, I don’t really have to test, but I think I’d like to commit on every change this afternoon, and I’d prefer to maintain the habit of running the “examples” (big twitter argument yesterday and today, are they tests or examples).

Running the tests and committing takes 20 or 25 seconds. Rather a long delay, but I’ll be patient. Takes me longer to write a paragraph, probably.

Now let’s change the signature to put verb in as the first parameter.

    private fun take(verb: String, noun: String, world: World): String {
        world.take(noun)
        world.response.say("$noun taken.")
        return roomName
    }

IDEA highlights all the lines that need to be changed.

            "take axe" -> take(cmd, "axe", world)
            "take bottle" -> take(cmd, "bottle", world)
            "take cows" -> take(cmd, "cows", world)

If there are others, the tests will find them. Probably there is some other way to spot the problems if there are any. We are green. Commit: new signature for take.

Now the same for move, slightly different layout. Rename direction to verb this time:

    fun move(verb: String, world: World): String {
        val (targetName, allowed) = moves.getValue(verb)
        return if (allowed(world))
            targetName
        else
            roomName
    }

Test, commit: renamed direction to verb in move.

Change signature:

    fun move(verb: String, noun: String, world: World): String {
        val (targetName, allowed) = moves.getValue(verb)
        return if (allowed(world))
            targetName
        else
            roomName
    }

And the callers:

    fun command(cmd: String, world: World) {
        val name = when(cmd) {
            "take axe" -> take(cmd, "axe", world)
            "take bottle" -> take(cmd, "bottle", world)
            "take cows" -> take(cmd, "cows", world)
            "inventory" -> inventory(world)
            "s","e","w","n" -> move(cmd, "", world)
            "xyzzy" -> move(cmd, "", world)
            "cast wd40" -> castSpell("wd40", world)
            else -> "unknown cmd $cmd"
        }
        world.response.nextRoomName = name
    }

My plan, in case it isn’t clear, is to get all these calls set up so that I can call them all the same way.

As I look at it now, this is not a good idea. It’s too soon. I’m concerned that I won’t be able to sort this out. We’ll see. I might just make it work. To do so, I’m going to have to pass the command cmd, which is all I really have, into all the methods, as both verb and noun. We’ll see how that goes. Let’s continue to make all the sequences the same.

    private fun inventory(verb: String, noun: String, world: World): String {
        world.showInventory()
        return roomName
    }

            "inventory" -> inventory(cmd,cmd, world)

And finally …

    private fun castSpell(noun: String, verb: String, world: World): String {
        world.flags.get("unlocked").set(true)
        world.response.say("The magic wd40 works! The padlock is unlocked!")
        return roomName
    }

            "cast wd40" -> castSpell(cmd,"wd40", world)

Test. Green. Commit: inventory and castSpell follow verb, noun, world form.

Now, if I’m not mistaken, I can refactor to return the functions and call them all the same way. I expect take to break.

    fun command(cmd: String, world: World) {
        val action: (String, String, World)->String = when(cmd) {
            "take axe" -> ::take
            "take bottle" -> ::take
            "take cows" -> ::take
            "inventory" -> ::inventory
            "s","e","w","n" -> ::move
            "xyzzy" -> ::move
            "cast wd40" -> ::castSpell
            else -> ::unknown
        }
        val name = action(cmd, cmd, world)
        world.response.nextRoomName = name
    }

    private fun unknown(verb:String, noun:String, world: World): String {
        return "unknown cmd $verb"
    }

For some reason, the tests run. The take tests must be profoundly weak.

    private fun take(verb: String, noun: String, world: World): String {
        world.take(noun)
        world.response.say("$noun taken.")
        return roomName
    }

Right. Do I even check the response? Do I even have tests for take at all? Quite likely not. When I run the game, I do encounter the defect that I foresaw:

Welcome to Tiny Adventure!
You're in a charming wellhouse.
You find keys.
You find bottle.
You find water.

> take bottle
take bottle taken.

Take isn’t really even checking to see if you can take the thing. I need a test for it and then to make it work. Ah, look! I’m not quite as awful as I thought. I do have tests for take:

    @Test
    fun `world has inventory`() {
        val world = world {
            room("woods") {
                go("s","clearing")
            }
        }
        world.take("axe")
        world.take("bottle")
        assertThat(world.inventoryHas("axe"))
        val game = Game(world,"woods")
        game.command("inventory")
        val s: String = game.resultString
        assertThat(s).contains("axe")
        assertThat(s).contains("bottle")
        assertThat(s).isEqualTo("You have axe, bottle.\n\n\n")
    }

How does that even work? What does World do with take?

Oh. It just does this:

    fun take(item:String) {
        inventory += item
    }

The intention here is that the take command will tell the world what to take. And I think the name take here is confusing. Should be addToInventory perhaps. Let’s do that.

OK that makes it more clear that the test we have is just testing inventory addition. But it would be better if this test were to actually execute a take command, and that should still break.

I think it better if I write a new test for this … and after some difficulty, I have one, but I hate it:

    @Test
    fun `take command works`() {
        val world = world {
            room("woods") {
                item("axe")
            }
        }
        val room = world.unsafeRoomNamed("woods")
        val response = GameResponse()
        response.nextRoom = room // hackery
        world.response = response
        room.command("take axe", world)
        assertThat(world.resultString).isEqualTo("")
    }

I hate it because I had to set response.nextRoom or suffer a diagnostic that nextRoom wasn’t set. There’s something not right about how I’m initializing the world. However, with this test in place, I do get an error and it confirms what we already knew:

Expecting:
 <"take axe taken.

You find axe.
">
to be equal to:
 <"">
but was not.

We’re getting the “take axe taken.” message, that should say “axe taken”. In addition, the axe is still there, but that’s no surprise. I am also a bit surprised about the message because there’s no room description in it, but I see that I didn’t provide one. Let’s change the test:

        val world = world {
            room("woods") {
                desc("You are in the woods.", "You are in the dark woods.")
                item("axe")
            }
        }

OK, that should make the error worse.

Expecting:
 <"take axe taken.
You are in the dark woods.
You find axe.
">
to be equal to:
 <"">
but was not.

Perfect. Now let’s improve the expectations on the test.

        room.command("take axe", world)
        assertThat(world.resultString).contains("You are in the dark woods.\n")
        assertThat(world.resultString).doesNotContain("take axe taken")
        assertThat(world.resultString).contains("axe taken")
        assertThat(world.resultString).doesNotContain("You find axe.")

That should give me some very nice errors. Hey! Does this thing stop on the first assertion that fails? How bogus! Anyway:

Expecting:
 <"take axe taken.
You are in the dark woods.
You find axe.
">
not to contain:
 <"take axe taken">

We’ll fix that one now, in take, then probably call it an afternoon.

    private fun take(verb: String, noun: String, world: World): String {
        // interim hackery waiting for new parser
        val words = noun.split(" ")
        val realNoun = words.last()
        world.addToInventory(realNoun)
        world.response.say("$realNoun taken.")
        return roomName
    }

Here I know that we are really seeing the whole command “take axe” or whatever, so I rip off the last word and just take that. That might actually work even after we fix the parsing. It works for now and gives me this error:

Expecting:
 <"axe taken.
You are in the dark woods.
You find axe.
">
not to contain:
 <"You find axe.">

Now what we should be doing is first, checking to be sure the room does have an axe, and second, removing it from the room if we take it. Let’s see about that.

When we add an item, we just do this:

    fun item(thing: String) {
        contents+=thing
    }

So we should be able to do something like this:

    private fun take(verb: String, noun: String, world: World): String {
        // interim hackery waiting for new parser
        val words = noun.split(" ")
        val realNoun = words.last()
        val done = contents.remove(realNoun)
        if ( done ) {
            world.addToInventory(realNoun)
            world.response.say("$realNoun taken.")
        } else {
            world.response.say("I see no $realNoun here!")
        }
        return roomName
    }

Now let’s run that test again. It passes. Let’s try one more thing:

    @Test
    fun `take command works`() {
        val world = world {
            room("woods") {
                desc("You are in the woods.", "You are in the dark woods.")
                item("axe")
            }
        }
        val room = world.unsafeRoomNamed("woods")
        val response = GameResponse()
        response.nextRoom = room // hackery
        world.response = response
        room.command("take axe", world)
        assertThat(world.resultString).contains("You are in the dark woods.\n")
        assertThat(world.resultString).doesNotContain("take axe taken")
        assertThat(world.resultString).contains("axe taken")
        assertThat(world.resultString).doesNotContain("You find axe.")
        val r2 = GameResponse()
        world.response = r2
        r2.nextRoom = room
        room.command("take axe", world)
        assertThat(r2.resultString).contains("I see no axe here!\n")
    }

When we try again to take the axe, we should get the “I see no axe here” message. (One could argue for “you already have it” but we’re incrementing here.)

Test runs. Commit: take command improved to check room for having the item and moving it to inventory, removing from room.

Let’s sum up. There’s salmon for supper.

Summary

So. In preparation for a better parser, I’ve converted all the command sub-methods to accept the same calling sequence (although they all use it but weakly so far). I’ve arranged to return the method to be called, rather than to call it in the when block, which ensures that I can’t so often forget to include the world, and that I won’t have to type it so often.

And I’ve enhanced take so that, in a somewhat kludged fashion, it actually figures out what you’re asking for and moves it from the room to your inventory if it’s present in the room.

All the changes were small, and the only real difficulty was in setting up the world test. There’s something odd about my late initialization by singleassign. I think Hill told me that he didn’t like that. Maybe he was right not to. We’ll explore that another time.

I have looked upon the world and seen that it is, if not good, then better.

See you next time!