Kotlin 66: Right-Timing

GitHub Repo

Some things seem to me to be happening at the wrong time. One issue is the GameResponse. Another is in building the world. What ever shall I do?

Despite my feeling that this program is “almost done”, I think there is plenty left to learn, including some infelicities¹ in the code, and the possibility that a game designer would want to do some reasonable thing that isn’t at all easy to do. Notions include:

1. The GameResponse is set into the world manually, at least in the tests, which need access to it to check things. I suspect it should be created at the beginning of command and returned from the command.

2. I can imagine at least one case where the game’s room description might want to include something like “There is a locked gate to the west”, unless the gate is unlocked, in which case it might mention an unlocked gate. We can’t quite do that with our current list of items in the room, which always says “There is a thing here.”

3. The issue above makes me think that we need to build enough with the DSL to determine how well it fits actual need. A lot of what I’ve done has been speculative, based primarily on my desire to learn Kotlin rather than driven by specific world builder needs.

4. Along the same lines, I think we need to check, and probably enhance the returned Phrase from the PhraseFactory (remind me to rename that). I think we might need to encode whether the verb or noun are known words or unknown. Again, some experiments with what the world builder wants the game to do are in order.

5. World-building itself happens oddly. When we start building the world, the instance gets fully initialized with the official game versions of the Lexicon and Actions. In the tests, these are generally not what we want, and the tests set up their own Lexicon and Actions. Perhaps the World should be given those, or perhaps they should be set up fully dynamically. I don’t know: we need to look.

General Remarks

I’ve commented that I think we’re done here, so why is this going on? There are at least a couple of reasons. First, we don’t know whether it’s possible to build a decent game with the features we have. Second, any code is suitable for me to learn from and write about, because there’s always something to improve or explain.

So I’m OK with continuing in this code base for a while.

GameResponse

I think this morning I want to start with the GameResponse. This is in part due to the fact that setting up tests can be really awkward. Now, partly, the awkwardness may be due to the fact that early tests didn’t have enough infrastructure to build on. Another likely cause is that my unfamiliarity with Kotlin caused me to write inferior code. Another is that I rarely write the best possible code right out of the box. My whole style is built around improving existing code, and I offer to you the possibility that much of your life is the same.

Anyway the response. Let’s see where it comes from and where it goes.

class GameResponse {
    var sayings = ""
    lateinit var nextRoomName: String
    var nextRoom: Room by singleAssign()
    val resultString: String get() = sayings + nextRoom.longDesc +"\n"+ nextRoom.itemString()

    fun say(s:String) {
        sayings += s+"\n"
    }
}

Already, things to question. First, there is a var member. It’s a string and we append to it. There are alternatives, including a string builder, which I’m sure Kotlin could provide, or a mutable list of Strings. Second, the lateinit is a serious code smell. The declaration is new to me in Kotlin, so at first I didn’t understand why GeePaw Hill objected to it when he first saw me using it.

The thing is this: the presence of a lateinit variable tells us that, as created, the object is not complete. And creating half-baked objects can lead to trouble. And … in fact we’ve seen trouble from other uses of lateinit, where tests have failed because something wasn’t initialized.

There is a pattern, called “Complete Constructor” by Kent Beck, that suggests that whenever we create an object, we create it completely, ready to be used. Clearly, we’re not doing that here.

Now I suspect that whenever we see a late init, we are looking at am object that is being created at the wrong time. That’s the case here. Our tests create a response and pass it in, so that they can check it later. Here’s an example.

Nice IDEA Feature

I want to mention a nice IDEA feature here. If I click on the class GameResponse and hit Command-B, IDEA pops up a list of all the usages of the class. Even better, there is a button on that popup that opens the list in the “Find Tool Window”, which opens on the bottom half of the screen and retains the whole list of the lines found, and even shows the project hierarchy leading to the line.

That allows me to look at a few choices, which is useful now, when picking an example to paste, but also useful when getting a general impression of how things are done or when looking for a likely place to start with refactoring or a change.

Nice feature!

Anyway, an example of creating a GameResponse just to have access to it:

    @Test
    fun `room go has optional block`() {
        val myWorld = world {
            room("first") {
                desc("first room", "the long first room")
                go("n", "second") { true }
                go("s","second") {
                    it.say("The grate is closed!")
                    false
                }
            }
            room("second"){}
        }
        val myRoom = myWorld.unsafeRoomNamed("first")
        val secondRoom = myWorld.unsafeRoomNamed("second")
        val response = GameResponse()
        val cmd = Command("s")
        myWorld.command(cmd, myRoom, response)
        assertThat(response.nextRoomName).isEqualTo("first")
        assertThat(response.sayings).isEqualTo("The grate is closed!\n")
        val r2 = GameResponse()
        myWorld.command(Command("s"), secondRoom, r2)
        assertThat(r2.nextRoomName).isEqualTo("second")
    }

Here we create two responses and do the World.command, which is rigged to accept a new response.

Clearly this code is saying that whoever’s sending this command wants to know the response to it. That being the case, it would probably be better if the command just returned it.

How does the game itself do it? Ah, it’s weird here too. When there is a command typed in, the Player is given the command:

    fun command(commandString: String) {
        response = GameResponse()
        world.command(Command(commandString), currentRoom, response)
        currentRoom = response.nextRoom
    }

Finally, let’s look at what World.command does:

    fun command(cmd: Command, currentRoom: Room, newResponse: GameResponse) {
        response = newResponse
        currentRoom.command(cmd, this)
        response.nextRoom = roomNamedOrDefault(response.nextRoomName, currentRoom)
    }

Ah. There’s another issue hiding here. The World does not know the current room: it’s passed in to command. The rationale for this was the possibility that there could somehow be more than one player in the World at the same time, and that each of them could have their own current room.

This was just speculation, and frankly I think that whoever² made this accommodation for a feature that might never exist made a mistake.

But wait, as long as we’re looking at this aspect of our design, there’s more! The Room does the actual command, as we can see above. And it is not passed the response. Instead, because it has access to world when its commands are done, it refers to the world to get the response. For example:

    val take = { imperative: Imperative, world: World ->
        val done = contents.remove(imperative.noun)
        if ( done ) {
            world.addToInventory(imperative.noun)
            world.response.say("${imperative.noun} taken.")
        } else {
            world.response.say("I see no ${imperative.noun} here!")
        }
    }

We could, and perhaps should, cover that reference. Or, we could cache the response in the room. Dealer’s choice, I guess. I have no strong preference just now.

Let’s keep our eye on the response. Let’s return it from the world command method.

Or …

I’ve thought of an alternative … we do allow the room to access our response. We could extend that privilege to the tests as well. Demeter³ demurs. We’ll return it.

I type this:

    fun command(cmd: Command, currentRoom: Room, newResponse: GameResponse) {
        response = newResponse
        currentRoom.command(cmd, this)
        response.nextRoom = roomNamedOrDefault(response.nextRoomName, currentRoom)
        return response
    }

Idea underlines the last word and offers the option to change the type of the enclosing function. I accept its offer and get:

    fun command(cmd: Command, currentRoom: Room, newResponse: GameResponse): GameResponse {
        response = newResponse
        currentRoom.command(cmd, this)
        response.nextRoom = roomNamedOrDefault(response.nextRoomName, currentRoom)
        return response
    }

Test. Green, of course. Commit: World.command returns response.

Now let’s find all the senders of this method and see whether they could be satisfied with the returned response. (I can’t think why they wouldn’t.)

class Player ...
    fun command(commandString: String) {
        response = GameResponse()
        world.command(Command(commandString), currentRoom, response)
        currentRoom = response.nextRoom
    }

This method is caching the response so that it can know the current room. I think we will want to change that around a bit: I’m not sure it has any other purpose. But we can change what happens inside the method readily. Note that we’re still creating the response: we’ll try to resolve that once everyone is honoring the return.

    fun command(commandString: String) {
        response = GameResponse()
        response = world.command(Command(commandString), currentRoom, response)
        currentRoom = response.nextRoom
    }

That looks odd but harmless. Test anyway. I’ll have to run the game for this one, just to be sure. Green and game works. Maybe I would be happier with more tests at the Player level. Maybe there are even enough already. Anyway Commit: Player uses response returned from World.command.

Our next candidate is that test that we saw before:

    @Test
    fun `room go has optional block`() {
        val myWorld = world {
            room("first") {
                desc("first room", "the long first room")
                go("n", "second") { true }
                go("s","second") {
                    it.say("The grate is closed!")
                    false
                }
            }
            room("second"){}
        }
        val myRoom = myWorld.unsafeRoomNamed("first")
        val secondRoom = myWorld.unsafeRoomNamed("second")
        val response = GameResponse()
        val cmd = Command("s")
        myWorld.command(cmd, myRoom, response)
        assertThat(response.nextRoomName).isEqualTo("first")
        assertThat(response.sayings).isEqualTo("The grate is closed!\n")
        val r2 = GameResponse()
        myWorld.command(Command("s"), secondRoom, r2)
        assertThat(r2.nextRoomName).isEqualTo("second")
    }

We just need to save the returns from the call to command. Let’s use new words:

        val myRoom = myWorld.unsafeRoomNamed("first")
        val secondRoom = myWorld.unsafeRoomNamed("second")
        val unused = GameResponse()
        val cmd = Command("s")
        val resp1 = myWorld.command(cmd, myRoom, unused)
        assertThat(resp1.nextRoomName).isEqualTo("first")
        assertThat(resp1.sayings).isEqualTo("The grate is closed!\n")
        val unused2 = GameResponse()
        val resp2 = myWorld.command(Command("s"), secondRoom, unused2)
        assertThat(resp2.nextRoomName).isEqualTo("second")

I’m tempted to add a TODO, but I hate⁴ them and also don’t know just what they do. Time to learn, I guess. Let’s do it.

        val unused = GameResponse()
        TODO("remove unused responses when possible")

If this works nicely, it might help with the final cleanup. We’ll see. Where else do we create these deals?

Arrgh. No. In IDEA, TODO amounts to an error. That won’t serve me at all. Remove it.

    @Test
    fun `room action`() {
        val world = world {
            room("busy") {
                desc("you are in a busy room", "you are in a very busy room")
                action(Phrase("look","around"),
                    {imp-> imp.world.say("Lots happening")})
            }
        }
        val room = world.unsafeRoomNamed("busy")
        val response = GameResponse()
        val command = Command("look around")
        world.command(command,room, response)
        assertThat(response.resultString).contains("Lots happening")
    }

I give it the same treatment:

    @Test
    fun `room action`() {
        val world = world {
            room("busy") {
                desc("you are in a busy room", "you are in a very busy room")
                action(Phrase("look","around"),
                    {imp-> imp.world.say("Lots happening")})
            }
        }
        val room = world.unsafeRoomNamed("busy")
        val unused = GameResponse()
        val command = Command("look around")
        val response = world.command(command,room, unused)
        assertThat(response.resultString).contains("Lots happening")
    }

I forgot to run the tests after the first change. I do so now. Green. Commit: Tests use command response, not their own. Prepping for removal of provided response.

A little quick searching tells me that a TODO comment will be shown in an IDEA search window. I put those in. That’s what I was looking for: a reminder, not a fatal error.

I think there’s one more case:

    @Test
    fun `unknown command`() {
        val world = world {
            room("woods") {
                desc("You are in the woods.", "You are in the dark woods.")
            }
        }
        val room = world.unsafeRoomNamed("woods")
        val response = GameResponse()
        val badCommand = Command("abcd efgh")
        world.command(badCommand, room, response)
        assertThat(response.resultString).contains("unknown command 'abcd efgh'")
    }

Same treatment:

    @Test
    fun `unknown command`() {
        val world = world {
            room("woods") {
                desc("You are in the woods.", "You are in the dark woods.")
            }
        }
        val room = world.unsafeRoomNamed("woods")
        val unused = GameResponse()
        // TODO remove unused when possible
        val badCommand = Command("abcd efgh")
        val response = world.command(badCommand, room, unused)
        assertThat(response.resultString).contains("unknown command 'abcd efgh'")
    }

Test. Green. Commit: Tests use command response, not their own. Prepping for removal of provided response.

IDEA reminds me “3 TODO”. Very nice. I push anyway.

Now I believe, but am not certain that World.command can create the response instead of using the one provided to it. We’ll try that and if the tests pass, carry on, otherwise figure out why my belief was mistaken.

    fun command(cmd: Command, currentRoom: Room, newResponse: GameResponse): GameResponse {
        response = newResponse
        currentRoom.command(cmd, this)
        response.nextRoom = roomNamedOrDefault(response.nextRoomName, currentRoom)
        return response
    }

Let’s just try creating one:

    fun command(cmd: Command, currentRoom: Room, newResponse: GameResponse): GameResponse {
        response = GameResponse()
        currentRoom.command(cmd, this)
        response.nextRoom = roomNamedOrDefault(response.nextRoomName, currentRoom)
        return response
    }

Test, hopefully: Green. Commit: World.command creates new GameResponse, response input parameter ignored.

Now we can change the signature of World.command:

    fun command(cmd: Command, currentRoom: Room): GameResponse {
        response = GameResponse()
        currentRoom.command(cmd, this)
        response.nextRoom = roomNamedOrDefault(response.nextRoomName, currentRoom)
        return response
    }

Test. Green. Commit: World.command no longer accepts unused third parameter.

IDEA offers warnings for all the unused names, which are presumably also indicated by my TODOs.

We’re clean and do a final Commit: Remove unused GameResponse creations.

Summary

We’ve completed a fairly significant change, from our command method being given a response to having it create its own and return it. We had to change something like 8 different places … and each change could have been done individually. I was impatient and did a few at a time, but even so, there were six separate commits, with full green bars every time. We could do this improvement over six days or six weeks, if our memory was that good. We could have improved things as we passed by the code, with essentially no impact on feature creation.

To me, this is the secret sauce of “Agile” software development, the creation of a way of working that lets us keep the code fresh and easy to modify, while all the time delivering new capability to our evil capitalist masters business-side colleagues.

We’ll ship this article and maybe do a new one. There’s certainly room for more improvement, and thus more learning, in this little program.

P.S.

I think I’ll start trying these TODO comments. I’d make yellow sticky notes for some of the things I notice, but often I put that off. Maybe TODO will help me remember things that need work. And … if I start getting hundreds of them, it’ll remind me why I don’t like them.

Good question though: would you rather see code needing improvement with a TODO, or code needing improvement without one?