The Repo on GitHub

I propose to make some changes that I think make sense and that my cohort will not disagree with much, if at all, despite not sharing my concern. As GeePaw put it last night, f[ool] around and find out.

What I propose, roughly, is to change the bot’s do_something method to have three phases, something like ‘update’, ‘decide’, ‘act’. This morning, at a guess, I’ll work on update, and maybe ‘act’.

I am doing this with an eye to client-server. We aren’t going to build out c/s yet, because we want to defer it to see what happens, and because we want to do some experiments to bring our understanding up a level or two. But we do know that we have to get to client-server, and I think that presently we are doing things that too severely limit our ability to do that.

In essence, I propose to refactor to remove those limitations. The refactoring will perforce include some new classes, I suspect. We’ll find out.

Added in Post
We do find out. Everything goes smoothly, and we do not get any new classes, and not much in the way of new methods, just a bit of rearranging and some new tests. And yet, I am now satisfied, my concerns alleviated.

Watch what happens, as a simple observation about a simple code change just smoothly wipes away my concerns about the design.

I have opened the zoom, but at 0800 that is an empty gesture, because on the current team I am the only one who is up and coding at 8 AM. Still, I would welcome them if they were to arrive, I can always use the help. Let’s get to it.

class Bot:
    def do_something(self):
        self.state()
        self.move()

The state member contains the state method which is to be executed as part of our current, um, state in the, um, state machine. It contains one of walking, looking, laden. The move call is an action that we always take: mo matter our state, we always try to move in our forward direction. move amounts to asking the World to move us.

The issue I’m trying to deal with is that move isn’t the only thing we currently ask the world to do as we execute our state machine, and our code is written as if we can access the results of that request immediately. For example:

    def looking(self):
        if self.can_take():
            self.take()
            if self.inventory:
                self.tired = 5
                self.state = self.laden

    def take(self):
        self.world.take_forward(self)

    def laden(self):
        if self.tired <= 0:
            if self.can_drop():
                block = self.inventory[0]
                self.world.drop_forward(self, block)
                if block not in self.inventory:
                    self.tired = 5
                    self.state = self.walking

These two operations, the take and drop, both check immediately after issuing the command to see whether the operation has worked. But only the world can decide whether the operation can actually take place. It is possible, for example, that the Bot has made a mistake and tried to do a take where there is nothing to take. It is possible that since the last time the Bot got a turn, some other Bot has taken what this one meant to take or dropped something where this one meant to drop its burden. In short, the operation requested might not work.

Now … it we were to assume that the message to the world suspends the Bot until the message and reply have completed a round trip, that code would be OK. And we surely could implement things that way, such that every time any Bot interacts with the world, we immediately alert the Internet and find out what happened. I think that would have two problems that concern me:

  1. It would increase the network traffic by a factor of ten or twenty over having each client buffer up all its residents’ commands and do them in a batch;
  2. If we have to wait for a network interaction on every such decision, it will take ages for our client to loop over all its bots and let them execute.
  3. (As so often happens, I was wrong about the “two”.) We might likely get into a very serious coroutine or multi-threading situation that would complicate our lives horribly.
Stumbling

My plan last night was “just” to add in the update phase and begin to make use of it. I am now thinking that it might be harder than I imagined. Oh heck, let’s just make sure we’re on a clean slate and try something. I push my little cs spike from yesterday and now we’re clean.

I am going to proceed without tests for at least a moment.

    def do_something(self):
        self.update()
        self.state()
        self.move()
        
    def update(self):
        pass

Unsurprisingly that works. Now what? Let’s look at the laden state again:

    def laden(self):
        if self.tired <= 0:
            if self.can_drop():
                block = self.inventory[0]
                self.world.drop_forward(self, block)
                if block not in self.inventory:
                    self.tired = 5
                    self.state = self.walking
Note
What follows is brilliant, in that just moving these lines of code essentially entirely addresses my concern that as written, client-server would cause us trouble. But it was not a sudden act of genius. It was just a very simple observation, oh, look, if we were to move this code …

The issue that I have with this is that it checks inventory right after issuing the command, and I believe that we can’t be sure we have an immediate return of the inventory change. But next time around, we can be sure, so let’s move the check for state change:

    def laden(self):
        if self.has_no_block():
            self.tired = 5
            self.state = self.walking
            return
        if self.tired <= 0:
            if self.can_drop():
                block = self.inventory[0]
                self.world.drop_forward(self, block)

Of course there is no such method, yet.

Grrr
I am more than a little irritated to find that all my tests are green, while this code clearly can’t even run. There are apparently no tests for the state machine. I will deal with that, but for now, let’s proceed to make this work.
    def has_no_block(self):
        for entity in self.inventory:
            if entity.name == 'B':
                return False
        return True

Tests are still green, as they just about have to be since we have no tests for this, but I run the game and the bots still pick up blocks, turn red, drop blocks, turn green. I conclude that this works.

Note that update takes no action at this time. It is just a placeholder. All the real updating is still taking place dynamically with the World sending messages directly to our Bot.

I am going to make a similar change to looking, which looks like this:

    def looking(self):
        if self.can_take():
            self.take()
            if self.inventory:
                self.tired = 5
                self.state = self.laden

Same kind of change:

    def looking(self):
        if self.inventory:
            self.tired = 5
            self.state = self.laden
            return
        if self.can_take():
            self.take()

Still green: still no tests. Try game again. Looks the same as ever. I could commit this, and I just might, but first I will try to expiate my various sins by writing some tests for the state machine. This might get tricky. I might even want a test double.

class TestBotStateMachine:
    def test_hookup(self):
        assert 2 + 1 == 4

Yay, a failing state machine test! We’re on the way. I want to test that the machine makes transitions when it should. Let me try a really invasive test and see how we like it.

    def test_laden_goes_to_walking_if_no_block_in_inventory(self):
        bot = Bot(5, 5)
        bot.state = bot.laden
        bot.state()
        assert bot.state == bot.walking

That test passes, as it should. The bot has no block and so it changes state. I’ll do a couple more.

After only a bit of grinding, I have these tests:

class TestBotStateMachine:
    def test_laden_goes_to_walking_if_no_block_in_inventory(self):
        bot = Bot(5, 5)
        bot.tired = 0
        bot.state = bot.laden
        bot.state()
        assert bot.state == bot.walking

    def test_looking_goes_to_laden_if_block_in_inventory(self):
        bot = Bot(5, 5)
        bot.tired = 0
        vision_list = [('R', 5, 5)]
        bot.vision = vision_list
        bot.state = bot.looking
        bot.state()
        assert bot.state == bot.looking
        bot.receive(Block(2, 2))
        bot.state()
        assert bot.state == bot.laden

    def test_laden_stays_laden_if_cannot_drop(self):
        # we call state() twice to ensure round trip update
        bot = Bot(5, 5)
        bot.tired = 0
        entity = Block(3, 3)
        bot.receive(entity)
        vision_list = [('R', 5, 5), ('B', 6, 5)]
        bot.vision = vision_list
        bot.direction = Direction.EAST
        bot.state = bot.laden
        bot.state()
        assert bot.has_block()
        assert bot.state == bot.laden
        bot.state()
        assert bot.has_block()
        assert bot.state == bot.laden

They’re all a bit nasty, especially that last one. It’s just that we need a lot of setup, because we need to have a block in inventory, a vision that shows a block where we want to put ours, and then we need to run the state twice, once to fail to drop and once to check that we don’t change state.

Just now, I don’t see how to make that simpler, as things stand. What if we were to refactor the actual state:

    def laden(self):
        if self.has_no_block():
            self.tired = 5
            self.state = self.walking
            return
        if self.tired <= 0:
            if self.can_drop():
                block = self.inventory[0]
                self.world.drop_forward(self, block)

With the need to return, I don’t see a way to break out something smaller and testable. We could perhaps do something with additional states. For example, we could set a state laden_expect_no_block after attempting the drop, and in that state either transition to walking or back to laden. That would cost us a full cycle around the loop. Maybe we would have methods we’d call from the various states, that we could test.

I don’t see anything desirable right now, and I think we should move the state machine off to a place of its own before making it more complicated, as it is just a set of methods inside the already fairly complicated Bot class.

I’ll leave this for another day. We are green, we have tests, and the game works as it did before, at least to the naked eye. By the rules of the cohort, I think I am allowed to commit this. I do want a review from my peers and betters.

Commit: change state machine so that looking and laden check inventory at top of method, not bottom. Preparation for client-server, call me paranoid. Added empty update method same-same.

Summary

I really thought that I was going to do things in the new update method, and that I might even need some kind of “results” object from server to client. We will surely need something like that someday, to pack and unpack the JSON messages or whatever we use, but I had thought I’d have to fake that a bit.

Instead, when I looked at the looking and laden states, I saw that if they would just check for having inventory at the beginning of the state rather than the end, that would mean that they did not check for a world status change right after requesting it, but the “next time around”, which completely settles my concerns.

I am not personally troubled at all when my plan going in falls by the wayside, replaced with something else. In my experience that happens very often. Kent Beck used to refer to “letting the code participate in the design decisions”, and that’s just what happens. When we look at the code in detail, we often see smaller steps that lead in the direction we want to go. And, often, that direction isn’t quite what we thought we would want when we only saw the larger picture.

Question
A question one might ask oneself in this light is: Just what value is there in committing to a detailed plan for a week or two, when, if we do our jobs well, we will deviate from that plan almost immediately on the first day?

I think the answer is that design discussion, sketching, planning have great value in preparing us to make good decisions, but that it is at best fraught and probably harmful to commit to the plan as it stands before we get started. We may have a goal. Today’s goal, for me, was really “do something to settle my concerns about client-server, because I believe as the code stands it can’t work well”. And that goal was entirely met by moving a few lines of code from the end of two methods to the beginning.

Planning is good. Believing the plan, committing to the plan? I think that’s imprudent.

Tension Resolved

I feel better about client-server now. I am convinced that by checking things at the top of a state method rather than at the end, we can allow for a batched round-trip rather than forcing an immediate return from a network call right in the middle of a Bot’s do_something. I am a bit frustrated that I’ve not been able to get my concern across to the cohort but we’ll get sorted out, I’m sure.

I needed the empty update method to set my mind in the mode of getting the current situation at the beginning of things, and someday we may need to fill that in (or do it for all the bots before cycling at all), but it opened the door to the “check-first” idea and two simple changes to the state machine completely eliminate my worries. Amazing!

I think the code is close enough to decent that there will be no major objections, and I am hopeful that we’ll find some ways to improve it.

The testing situation is unsatisfactory but at this moment I don’t see what to do about it. It seems that the Bots just need a lot of context to make their decisions. I’m hoping to get a group session working on this all together soon. In any case my current concerns are abated.

See you next time!