The Repo on GitHub

The top of our list needs attention. A small concern leads to much better code. Ron talks himself out of something. Comments are clues.

The list of things to do includes this item, which concerns me:

  • Prioritize error if can’t move, because _old_location is weird.

What this means is that we save an “old location” when we plan to step, and check it on the next go-round, to see if we have moved, and if not, we issue a turn command. But the code to set that location is out in the open and would be easy to get wrong. I think we’ll take a look at that.

Presently, the World only returns the Bot’s current location. If the bot tries to move and cannot, because it’s against the wall or another object, there is no indication given: it’s location just doesn’t change. Presently, the Bot caches its +old_location and checks to see if it has moved using that information. So the idea is “should we instead send an error back from World?”

Now, it would be perfectly reasonable to leave things as they are. The current rule is that when you try to step your Bot, if for any reason it cannot take that step, nothing happens. Simple, direct, leaves the concern up to the Bot programmer to deal with it. If the Bot programmer does not deal with it properly, the Bot will probably not work as the programmer intends.

So be it. The world just returns your state. It’s up to you to deal with it. So we have …

An Interesting little design problem

    def get_actions(self):
        actions = []
        actions += self.turn_if_we_didnt_move()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        self._old_location = self.location
        actions += ['step']
        return actions

    def turn_if_we_didnt_move(self):
        if self.location == self._old_location:
            return self.change_direction()
        else:
            return []
Added in Post
My g*d, look how messy that get_actions method is! We have to actually think about it to understand it! Is this what Hill meant by “sloppy code”? I’m really glad now that we did what you’re about to read. Carry on …

Now our particular bot design, at present, is that we always try to take a step. So we always save our old location, and next time around, we always check it. If it is the same as it was, we change direction.

The code does not express that idea very well. It’s not terrible, at least the turn_if_we_didnt_move is pretty evocative, but it’s not great. What could go wrong?

  1. Suppose we change strategies so that sometimes we do not step. Perhaps we see a block to our left and our new strategy is to turn and pick it up. We’ll probably turn OK and pick up OK, but then next time around we’ll probably throw in another turn, which could leave us disoriented.

  2. Suppose we change our state machine so that sometimes it takes a step, and we forget to set the old location. We’ll get a turn at the beginning of our next, um, turn, whether we need one or not.

There might be more ways to go wrong, but two is enough to give us concern. How can we best protect ourselves?

  • We could protect ourselves by being more careful that the old location is updated when, and only when, we expect a location change. We might have a method called something like take_step that does the save and returns the ‘step’ action. But that doesn’t prevent someone from writing a new method that creates its own step action. “Hey, everyone, be careful in the future” isn’t a very strong strategy.

  • Maybe we could check the actions at the end of the get_actions method and if they contain a ‘step’ operation, save the old location. That would be a bit expensive, but probably only a couple of lines of code. It would be safer than what we have now, and safer than the preceding idea.

  • Could we determine the “didn’t move” fact differently? What about when the results come back from the World? The world sends back our status dictionary.

That last one is interesting. What do we do with the results?

Note
What follows is a short look at a design idea that we consider and then reject. It happens. It’s a good thing.
class Bot:
    def update(self, result_dict):
        self._knowledge.update(result_dict)

class Knowledge:
    def update(self, update_dictionary):
        self.direction = update_dictionary['direction']
        self.id = update_dictionary['eid']
        self.location = update_dictionary['location']
        self.receive(update_dictionary['held_entity'])
        self._scent = update_dictionary['scent']
        self.vision = update_dictionary['vision']

We could certainly detect that location was changing here. But, as things stand, we don’t know whether we intended to move or not. If we didn’t intend to move, the location won’t change, but we do not wish to invoke the turn.

It seems as if the best we can do is to be careful. We really only want to change direction if we intended to take a step and failed. So this idea won’t help.

Note
We explored the design idea and decided against it. Interesting idea but not for now.

If that’s the case then I see only two reasonable options:

  1. Keep things roughly as they are, with better encapsulation, and hope that no one issues a ‘step’ without setting the old location;
  2. Check the actions for ‘step’ and set the old location if there is one.

The second option seems safer. Let’s see how hard it would be to do.

    def test_bot_sets_old_location(self):
        bot = Bot(10, 10)
        assert bot._old_location is None
        actions = bot.get_actions()
        assert 'step' in actions
        assert bot._old_location == Location(10, 10)

This test kind of gives us a clue as to how to implement the feature. The actions we produce in get_actions are just simple strings. So we can change get_actions like this:

    def get_actions(self):
        actions = []
        actions += self.turn_if_we_didnt_move()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        actions += ['step']
        if 'step' in actions:
            self._old_location = self.location
        return actions

This looks pretty silly, and a sufficiently clever programmer might decide to optimize that right out. Let’s refactor so that it’s a bit more clear what’s going on.

    def get_actions(self):
        actions = []
        actions += self.deal_with_failed_intentions()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        actions += ['step']
        self.record_intentions(actions)
        return actions

    def record_intentions(self, actions):
        if 'step' in actions:
            self._old_location = self.location

    def deal_with_failed_intentions(self):
        if self.location == self._old_location:
            return self.change_direction()
        else:
            return []

We are green. I’ll commit and then we’ll discuss. Commit: create notion of recording and checking intentions.

Reflection

I feel semi-good about this. I think it’s close to a good idea. I’m tempted to put comments on those two new methods saying what they are for.

This is a clue that the method names are not bearing enough weight yet. The way I usually work out names is to talk with my team members about what’s going on, trusting that a decent phrasing will probably come up … and if not, well, we have these names and we could comment if we really need to.

What are we doing here? At the bottom of the method, we’re recording details of our plan, and later, at the top, next time around, we’re adjusting for differences between expectations and reality. What if we called them record_expectations and check_expectations? That seems better than intentions. Done. Now let’s look at what the code does under those names:

    def record_expectations(self, actions):
        if 'step' in actions:
            self._old_location = self.location

    def check_expectations(self):
        if self.location == self._old_location:
            return self.change_direction()
        else:
            return []

Would that be better if we saved and checked the expected location? I think it might, but computing the expected location isn’t easy. In principle, there could be multiple turns and steps in there, if the World allows them, which, presently, it does.

Hm! There is a potential bug in the old location scheme, if multiple steps are allowed. We could imagine a plan where we take two steps forward, take, turn around, take two steps back, winding up where we were but holding something.

I think that just means that record_ and check_expectations need to be suitably robust compared to one’s overall Bot strategy. I think we’ll let this ride. Commit: rename intentions to expectations.

More Reflection

All this amounts to a very small change in response to a small chance of a future mistake. One could imagine letting it slide. But I think it deals with a very valid concern, which is that some future change has a good chance of breaking the connection between what we think is going to happen and what does happen, and in the prior code, we had just one specific check that was rather fragile. Now we have a more explicit place for all such checks. That is more likely to give us cause to think carefully about other expectations, and about breaking the current ones.

So that’s good, I think, and I would suggest that in a real application, this kind of small change is worth making, because months from now, we’ll need all the help we can get. While we’re here, therefore, let’s

Review the Code

    def get_actions(self):
        actions = []
        actions += self.check_expectations()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        actions += ['step']
        self.record_expectations(actions)
        return actions
Added in Post
He says “a bit ragged”! This code is fugly! It’s hard to follow. It demands thinking that we cannot spare. Fortunately, he’s going to fix it. Read on.

This method is a bit ragged. Ideally, we’d like a method to consist of a series of operations all at the same level, or to contain one loop or one decision. We don’t adhere to that preference as well as we might, but this one could use a little improvement. Like this:

    def get_actions(self):
        actions = []
        actions += self.check_expectations()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        actions += self.possibly_change_direction()
        actions += ['step']
        self.record_expectations(actions)
        return actions

    def possibly_change_direction(self):
        if random.random() < self.direction_change_chance:
            return self.change_direction()
        else:
            return []

Getting closer. One more:

    def get_actions(self):
        actions = []
        actions += self.check_expectations()
        actions += self.updated_state_action()
        actions += self.possibly_change_direction()
        actions += ['step']
        self.record_expectations(actions)
        return actions

    def updated_state_action(self):
        self.state = self.state.update(self._knowledge)
        return self.state.action(self._knowledge)

Reflection

It seems to me that the above is far better than what we had when we came in. It looks more clean. It does similar things similarly. It hides decisions inside meaningful names. The individual methods, as shown above, all encapsulate one very simple idea.

Summary

We began with a valid concern: the handling of old location was just inserted in a couple of judiciously-chosen locations in the code. Those insertions were fragile: it would be easy to enhance the system so as to subvert the old location logic. We considered a few solutions and came up with the notion of “expectations”, and provided a method to set them and a method to check them. The methods are both trivial … but they stand in exactly the right place for a more robust set of expectations if we want them. As such, they protect the single expectation we currently support, and they will attract any future expectations that we want to set and check.

This was a very small but quite nice change: a well-named encapsulation of an important notion. We might have stopped here and done some good.

But then we saw that the get_actions method was itself rather messy. We refactored to make it far simpler. In essence, we applied the Composed Method pattern.

The resulting method isn’t entirely absolutely perfect as it stands. Perhaps we’ll fiddle with it in another session, but in my view it is “much improved” and even “pretty darn good”, if not “absolutely perfect and in accord with all the good advice we’ve ever had”.

We have left the campground more safe than it was when we came here, and better looking as well. We are pleased.

See you next time!