GitHub Repo

Let’s fix one of those ‘lateinit’ variables. They’re definitely a code smell.

Let’s look at lateinit. I am sure I have at least one, maybe more up in this thing. It’s a sure sign of incomplete objects and it’s an exception waiting to happen. No one wants an exception.

Here’s one:

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

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

I just put that TODO in a few minutes ago, back in article 66. (Hmm, that sounds familiar …) Presently we set that variable here:

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

    val move = { imperative: Imperative, world: World ->
        val (targetName, allowed) = moves.getValue(imperative.noun)
        if (allowed(world)) world.response.nextRoomName = targetName
    }

We now create the GameResponse in a few different places, perhaps too many. Let’s review that first.

The player wants to cache the Response. Maybe it shouldn’t. Here’s the whole class:

class Player(private val world: World, startingName: String) {
    private var response: GameResponse = GameResponse()
    var currentRoom = world.unsafeRoomNamed(startingName)
    val roomReferences: Set<String> get() = world.roomReferences

    val currentRoomName get() = currentRoom.roomName
    val resultString: String get() = response.resultString

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

That created value is never used. It’s private and it’s overridden by command. It’s just there because I didn’t want a nullable variable. But wait. Do we need it at all? What we really want to do is to cache the currentRoom. Let’s remove the member and make it local to command:

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

Test. Oops, resultString. Let’s cache that too.

class Player(private val world: World, startingName: String) {
    var resultString: String = ""
    var currentRoom = world.unsafeRoomNamed(startingName)
    
    val currentRoomName get() = currentRoom.roomName
    val roomReferences: Set<String> get() = world.roomReferences

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

This is marginally better, but we’re just caching that resultString so that whoever called this method could get at it. Let’s see who’s calling. There should be few.

Well, more than “few”. One call in the game and 18 tests.

Here’s the game one:

    private fun someoneTyped() {
        val cmd = myCommand.text
        player.command(cmd)
        myText.appendText("\n> $cmd")
        myText.appendText("\n"+player.resultString)
        myCommand.text = ""
        myCommand.appendText("")
    }

It’d be happy just to get the string back. Change it.

class Player(private val world: World, startingName: String) {
    var currentRoom = world.unsafeRoomNamed(startingName)

    val currentRoomName get() = currentRoom.roomName
    val roomReferences: Set<String> get() = world.roomReferences

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

And in the game:

    private fun someoneTyped() {
        val cmd = myCommand.text
        val resultString = player.command(cmd)
        myText.appendText("\n> $cmd")
        myText.appendText("\n"+resultString)
        myCommand.text = ""
        myCommand.appendText("")
    }

Test, expecting chaos. Innumerable tests fail for want of resultString. Fix them like this one:

        val player = Player(world, "first")
        player.command("s")
        assertThat(player.resultString).isEqualTo("long second\n")
        player.command("s")
        assertThat(player.resultString).isEqualTo("long second\n")

That becomes:

        val player = Player(world, "first")
        var resultString = player.command("s")
        assertThat(resultString).isEqualTo("long second\n")
        resultString = player.command("s")
        assertThat(resultString).isEqualTo("long second\n")

I can’t test because Kotlin insists that I fix them all. OK, fine, be that way.

All changed, just a couple of minutes. Green. Commit: Player command returns result string, no longer caches it.

Pop

Now, popping my stack, I recall that I was concerned about the lateinit here:

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

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

The idea would be for the nextRoomName to be provided when we construct the Response, since we know the current room, at least in World.command:

Who’s poking at this variable anyway?

Ah, perfect. Only this code sets it:

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

Therefore, let’s set it on construction, to the currentRoomName, in World:

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

That, plus the corresponding change in GameResponse, seems to break too many things. I’ll roll back and try again.

As before …

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

But this time I’ll default it if it’s not provided:

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

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

Test. Yes, green. I still don’t love this but it passes all the tests. Commit: lateinit removed from var nextRoomName in GameResponse.

Let’s do review all the users of this constructor.

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

World gives itself a response when it inits. I think this is just there to avoid yet another lateinit, although I recall vaguely that it may also have created a problem somewhere amid the rather messy tests. Let’s dig on …

    @Test
    fun `response has say method`() {
        val r = GameResponse()
        r.say("One thing")
        r.say("Another thing")
        assertThat(r.sayings).isEqualTo("One thing\nAnother thing\n")
    }

This is OK, I just used it to drive out the say method. But let’s put a value in there:

    @Test
    fun `response has say method`() {
        val r = GameResponse("ignored")
        r.say("One thing")
        r.say("Another thing")
        assertThat(r.sayings).isEqualTo("One thing\nAnother thing\n")
    }

And then …

    @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(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(Command("take axe"), world)
        assertThat(r2.resultString).contains("I see no axe here!\n")
    }

I think we can avoid a lot of rigmarole here if we use World.command. Let’s try, but first, let’s commit what we have, assuming we’re green. Commit: providing “ignored” parameters in GameResponse creations.

Now in that test above:

    @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 = world.command(Command("take axe"), room)
        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 =world.command(Command("take axe"), room)
        assertThat(r2.resultString).contains("I see no axe here!\n")
    }

Green. Commit: Remove unneeded creations of GameResponse. Use the one returned from world.command.

Reflection

We’ve removed the lateinit from GameResponse. Along the way we made several tests simpler, and enabled further similar tests to be created much more readily. A good payoff for understanding, for reliability, and for future work.

Nice. Push the article. See you next time!