GitHub Repo

Further thoughts on yesterday’s transformation, followed by looking for similar opportunities.

Over the past few days, we’ve moved from a single action method in Room, to both Room and World having three action functions each, similar but differing in calling sequence. Then, observing this blatant duplication, which is always, or so nearly always as to make no difference, a sign of some new thing needing to be born. In our case, the thing had been born: the Actions object. But it had not matured. As the repository for the game’s actions, it had the ability to find things, but not to add or remove things from the collection.

So, next, we moved the map-manipulating implementations of action, action, and action over to Actions — kind of a clue there, in the name — with forwarding calls for all three, in each or Room and World.

Then, after some research and experimentation, I learned how to write this:

interface IActions {
    fun act(imperative: Imperative)
    fun action(verb: String, noun: String, action: Action)
    fun action(verb: String, action: Action)
    fun action(commands: List<String>, action: Action)
    fun add(phrase: Phrase, action: (Imperative) -> Unit)
    fun clear()
}

class Actions() : IActions { ...

With the methods we wanted to have in World and Room defined in the interface, we could then, finally, say this:

class Room(
		val roomName:R, 
		private val actions:IActions = Actions()
		): IActions by actions {

What that means, in words that I can finally understand, is something like this:

I, the Room class, implement and have in myself, all the methods found in the interface IActions. Furthermore, they are implemented by my object actions, so please just forward any such calls to that object.

Once we have made said incantation, the Room class now implements all the methods in the interface, act, action, add, and clear.

I am assured by the people who assure me of things that this is exactly the purpose of the by keyword of Kotlin, when used in this context. (There is at least one other usage, which I do not as yet have under my belt.)

Enough history, did you have a point?

Yes, thanks, I did have a point, and it was that from days ago, we had an indication that things weren’t quite right. We had this:

class Room(val roomName:R) {
	...
    private val actionMap = mutableMapOf<Phrase,Action>(
        Phrase() to {imp -> imp.notHandled()}
    )
    private val actions = Actions(actionMap)

    // DSL Builders

    fun action(verb: String, noun: String, action: Action) {
        action(Phrase(verb,noun), action)
    }

    fun action(phrase: Phrase, action: Action) {
        actionMap[phrase] = action
    }

If one were to read that code in a suspicious tone of voice, one might say something like this:

OK, so this guy creates an action map, just a map object, with a phrase in it. Then he creates a member variable with that map inside it. Then, later on, he modifies that map that he gave away, WHAT KIND OF [DELETED] IS THIS???

Why is this so bad? Well, we know that what the program clearly intends in the action method is that the change to its local copy of the map is supposed to affect the Actions table. If we doubted that, we could dig through some code and tests and see that the find command in Actions only finds useful things because the assignment above puts them into its map from the outside!.

I’m not saying that’s the worst code I’ve ever written, nor even the worst code I may write before I shuffle off this mortal coil, but I can tell you that it’s a contender. Suppose we thought of a more effective internal data structure for Actions, and instead of just saving the input map, we converted it to the better form. Then, suddenly, the program would stop working.

So, let’s just say that from the very first moment I wrote that code, there were clear signs that it wasn’t right.

Fortunately, by the time I added some more ways of adding things to Actions, and used them in two different classes, I was tipped off to do something. Even then, my motivation was really to eliminate the duplication between the two classes World and Room. I wasn’t focused on fixing the design error, just on removing duplication. Fortunately, I’ve learned that duplication nearly always covers a design error, and everything unrolled into a better situation.

Is there a major lesson here? Even a minor one? Well, some lessons are already mentioned in habits, including:

  • Watch for Feature Envy
  • Wrap Native Collections

I’m going to update Habits to mention duplication.

Moving Right Along

Let’s have a quick look around to see if there might be a bit of code to clean up, whether via removal of duplication, discovery of feature envy, or something else.

Here’s GameResponse:

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

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

I’m not sure about that singleAssign thing in nextRoom. It’s used properly, that is, as Kotlin intended, because the nextRoom is updated only once, and the singleAssign notation means that we only update it once. But we don’t really do it quite as neatly as all that.

Here’s the main application:

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

What happens during a command can modify the nextRoomName, like this:

    action("say", "xyzzy" ) {
        say("Swoosh!")
        response.nextRoomName = R.WoodsNearCave
    }

When any command wants to change the room we’re in, it does so by assigning something directly to nextRoomName in the Response. Another example:

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

Here are some issues with this:

  1. We can now get the Room directly from the room name, which is now an enum that has the Room inside it. So one of nextRoomName or nextRoom is redundant.
  2. Bashing a new value into the response is rude. We should send it a message saying setNextRoom() or something like that.

There may be more. Let’s see if we can improve this a bit. First, I’ll change nextRoom, since it caught my attention.

class GameResponse(var nextRoomName:R = R.Z_FIRST) {
    var sayings = ""
    val nextRoom: Room = nextRoomName.room
    val resultString: String get() = sayings + nextRoom.description() +"\n"+ nextRoom.itemString()

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

I am certain that this will work fine, so I test it. The tests identify that line in command where command updates the nextRoom, jamming a value into someone else’s innards. That line is now redundant, so I remove it and test again.

I slowly realize that I need nextRoom to be a function, not a static value.

class GameResponse(var nextRoomName:R = R.Z_FIRST) {
    var sayings = ""
    fun nextRoom(): Room = nextRoomName.room
    val resultString: String get() = sayings + nextRoom().description() +"\n"+ nextRoom().itemString()

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

That, plus changing the references to calls, gets me green. Now let’s fix that poking of new values into our var nextRoomName. I’ll declare it private and find the objections.

    fun setNextRoom(room: R) {
        nextRoomName = room
    }

The callers are:

    response.setNextRoom(R.WoodsNearCave)

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

    fun move(imperative: Imperative, world: World) {
        D.executeIfDirectionExists(imperative.noun) { direction: D ->
            val (targetName, allowed) = moves.getValue(direction)
            if (allowed(world)) world.response.setNextRoom(targetName)
        }
    }

I expect green. Some tests disagree:

        val resp1 = myWorld.command(cmd, myRoom)
        assertThat(resp1.nextRoomName).isEqualTo(R.Z_FIRST)
        assertThat(resp1.sayings).isEqualTo("The grate is closed!\n")
        val resp2 = myWorld.command(Command("s"), secondRoom)
        assertThat(resp2.nextRoomName).isEqualTo(R.Z_SECOND)

Let’s see. We can make that be readable but not writable. I do this:

class GameResponse(startingRoomName:R = R.Z_FIRST) {
    var nextRoomName = startingRoomName
        get() = field // used only by tests
    var sayings = ""
    fun nextRoom(): Room = nextRoomName.room
    val resultString: String get() = sayings + nextRoom().description() +"\n"+ nextRoom().itemString()

    fun setNextRoom(room: R) {
        nextRoomName = room
    }

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

Now the input var is just used to init our nextRoomName, which we can set using the setNextRoom function, and which anyone can read via the get. That’s just done by tests.

I think we should rename that setNextRoom method. If I try to rename it to setNextRoomName, Kotlin objects, as if that name is reserved, I suspect because of the member var named nextRoomName.

Let’s rename it to moveToRoomNamed. fine. Still green.

I see no reason for nextRoom to be a function. We can keep it up to date now. I come down to this:

class GameResponse(private var startingRoomName:R = R.Z_FIRST) {
    var nextRoomName = startingRoomName
        get() = field // used only by tests
    var nextRoom: Room = nextRoomName.room
        get() = field

    val resultString: String get() = sayings + nextRoom.description() +"\n"+ nextRoom.itemString()
    var sayings = ""

    fun moveToRoomNamed(roomName: R) {
        nextRoomName = roomName
        nextRoom = roomName.room
    }

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

I’m not entirely happy with this, even now, but I think all the complexity is now on the inside. The Response is a communications object, and, as such, it is referenced and updated over time, so it is inherently temporally coupled. However, I do not like the necessity to update the nextRoomName and nextRoom together. I do like the result, which is that people can access next Room as a property rather than via a function.

I might be wrong to like that. I think I am not entirely clear on whether Kotlin vars behave like C# properties, or not.

Anyway we are green. Commit: GameResponse refactored for greater safety and sensible use from outside. Still odd inside.

Let’s take one more look around, maybe find one more little thing to do.

I tidy a few things not worth mentioning. Then I find this:

    fun take(imp: Imperative, world: World) {
        with(world) {
            imp.noun.let {
                response.say(
                    when (contents.moveItemTo(it, inventory)) {
                        true -> "$it taken."
                        false -> "I see no $it here!"
                    }
                )
            }
        }
    }

How about that indentation? Let’s see if we can inline some of that to mutual benefit.

First:

   fun take(imp: Imperative, world: World) {
        with(world) {
            response.say(
                when (contents.moveItemTo(imp.noun, inventory)) {
                    true -> "${imp.noun} taken."
                    false -> "I see no ${imp.noun} here!"
                }
            )
        }
    }

Then:

    fun take(imp: Imperative, world: World) {
        world.response.say(
            when (contents.moveItemTo(imp.noun, world.inventory)) {
                true -> "${imp.noun} taken."
                false -> "I see no ${imp.noun} here!"
            }
        )
    }

Uh, right. I think I went overboard with the with and let. I did save some dots but the code above is simpler and seems clear enough. A bit less nesting might be a good thing. Test, commit: Tidy up deep nesting in take.

I also got a warning that the function clear was not used, so I removed it. Its purpose, as yet unused, is to clear the player’s inventory. Since we don’t even have drop, that function could only be harmful: stuff would just disappear.

Summary

Working through the GameResponse took a few steps, including converting nextRoom to a function and then back to a var. There may be something not right about this object, but its status as a chuck of things that need to be shared between World, Room, and Player makes it a bit odd. Maybe there’s something better to be done there, but most of the oddness is now packaged inside it.

Then, I scanned a lot of additional code, and I don’t see much beyond what we mentioned above, and a few small tidyings. Time to turn back to growing the world, to see what else pops out. I think that some world-ranging things will need better support than the current GameStatus object. We’ll work on that when we need it.

Learnings? Well, check out the Habits article, plus clearly I need to understand Kotlin properties better than I currently do.

I’ll study up. See you next time!