And Now ...

The Repo on GitHub

We set out to do a little code review, maybe some light refactoring. We get a surprise, thinking something is broken. It isn’t. We learn descriptors a bit better. We discover questionable code, no surprise there. We improve. We see more for next time. Standard day in every way.

We installed our new Forwarder descriptor in Bot. It looks rather nice, if I do say so:

class Bot:
    direction = Forwarder('_knowledge')
    holding = Forwarder('_knowledge')
    id = Forwarder('_knowledge')
    location = Forwarder('_knowledge')
    vision = Forwarder('_knowledge')

I wonder if that class would benefit from a better name. We could call it ForwardTo, or FetchFrom, or DelegateTo. It’s probably worthwhile to think about that, though if it were to be called DelegateTo, I’d want to be sure that it works properly for methods and properties and the like. Speaking of … what does the Knowledge class that feeds our Forwarder look like?

class Knowledge:
    drop_threshold = 4
    take_threshold = 7
    energy_threshold = 5

    def __init__(self, location, direction):
        # world write / client read only
        self.direction = direction
        self.id = None
        self.location = location
        self._held_entity = None
        self._scent = 0
        self._vision = Vision([], self.location, self.direction)
        # local Bot client-side info
        self._energy = self.energy_threshold

    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']

    @property
    def vision(self) -> Vision:
        return self._vision

    @vision.setter
    def vision(self, vision_list):
        self._vision = Vision(vision_list, self.location, self.direction)

    @property
    def holding(self):
        return self._held_entity
    ...

I feel confident that the three simple forwards, direction, id, and location surely work, but I wonder about vision and scent. I think I’d like to have some tests for those.

Surprise

I think I see a problem. That drives me to write some additional tests and to think a bit more deeply about descriptors. That’s a good thing. There is no problem, I was mistaken about that. That’s good too.

I try the game quickly. The bots run around, but I do not see them clumping up the colors as they are supposed to.

I think these Forwarders may have broken the game! This is not good. We need some tests.

I see two possible next steps. One would be to write a test against the Knowledge class, using Forwarders like those in Bot. The other would be to extend our existing tests to include properties like the ones in Knowledge.

I think I’ll do the latter: it seems more pure, and I think I need a simple pure situation to work with.

I extend the test’s InfoHolder, the object we forward to:

class InfoHolder:
    def __init__(self):
        self.info = "info from InfoHolder"
        self._held_entity = "held"
        
    @property
    def holding(self):
        return self._held_entity

And add a Forwarder to our other test class:

class NeedsInfo:
    info = Forwarder('extra_data')
    holding = Forwarder('extra_data')

    def __init__(self):
        self.extra_data = InfoHolder()

And finally, the test:

    def test_property(self):
        needy = NeedsInfo()
        assert needy.info == "info from InfoHolder"
        assert needy.holding == 'held'

I am surprised: the tests passes. It fails if I look for ‘heldx’, so I’m sure it’s actually working.

Now what? I’ll test a bit more just to be sure, but I think this is telling me that the Forwarder is not the problem if in fact the game is not working properly.

A quick check tells me that the game is OK. It turns out that we run it in two modes. In one mode, it makes 20 moves internally before displaying anything on the screen. In the other, it makes only one move per screen refresh. In the 20x mode, the clumps form pretty rapidly. In the 1x mode, not so much. It was in 1x mode. We’re OK! Whew.

But as long as we’re adding tests, let’s see if Forwarder handles methods. Should be an easy check:

    def test_method(self):
        needy = NeedsInfo()
        assert needy.square(2) == 4

class InfoHolder:
    def __init__(self):
        self.info = "info from InfoHolder"
        self._held_entity = "held"

    @property
    def holding(self):
        return self._held_entity

    def square(self, number):
        return number * number

class NeedsInfo:
    info = Forwarder('extra_data')
    holding = Forwarder('extra_data')
    square = Forwarder('extra_data')

    def __init__(self):
        self.extra_data = InfoHolder()

Green. That’s good news. Let’s commit: added tests for property and method to descriptor tests.

Reflection

The fact that I was so willing to believe that the Forwarders might have broken the game, and that I felt the need to test properties and methods, tells us that the Forwarder, as nice as it is, is pretty deep in the bag of tricks: I don’t understand the Python internals well enough to be absolutely sure what the Forwarder can and cannot do.

Let us reason together: The Forwarder descriptor with name foo is called whenever Python tries to fetch the foo attribute of the instance. Its job is simple: fetch the attribute named foo from the receiver instance. There is no indication given as to why the attribute is being fetched. The Forwarder doesn’t know or care what the attribute is, it just gets and returns it.

So it has to “just work”. And it seems that it does.

Weird morning so far

This morning has been odd so far. I really thought we’d be somewhere else right now, but what looked like a defect, but wasn’t, sent me down a path of exploration that resulted in a couple of valuable tests and a reflection about how descriptors work that gives a bit more confidence that they’re just what we thought they were.

Often, my morning does not unfold as I thought it would. I believe that this is quite often the case for many programmers, perhaps nearly all. It would be possible to ignore signals from the code and just plod ahead doing what we planned, but I question whether that’s ever ideal. Kent Beck used to say that we should let the code participate in our design sessions. I think that’s just right, and that in fact the code should participate in all our thinking about the program.

Fact is, my programming sessions are so often notably different from my thoughts going in that I hardly even notice any more. I always expect to discover what to do, just as much as plan ahead what to do.

Let’s move on. Let’s look beyond those descriptors. Let’s see what else is going on in Knowledge.

    def __init__(self, location, direction):
        # world write / client read only
        self.direction = direction
        self.id = None
        self.location = location
        self._held_entity = None
        self._scent = 0
        self._vision = Vision([], self.location, self.direction)
        # local Bot client-side info
        self._energy = self.energy_threshold

Notice that our Knowledge instance mostly contains information from the World, which we treat as read-only, since changing it won’t affect the world anyway. Only commands affect the world.

But there’s one additional attribute in _knowledge, that doesn’t come from the world: the energy. That’s used in the state machine. In essence, when the Bot picks up a block, or drops it, its energy is used up, and until it regains energy, it just wanders around without looking for things to pick up or drop. It gains energy on each step, so after a while, its energy is back and it will do a pickup or drop-off as indicated. The effect of this is that the Bot tends to wander away from where it picked up a block, so it isn’t likely to just put it back down where I got it.

And that’s enough to create clumps.

But … is it appropriate to keep the energy in the Knowledge? One could argue that we should keep the world info separate from the Bot’s own local info. In fact, my colleagues have made just that argument. I see the point but do not see that the value of that separation is worth the additional complexity. I could be wrong.

Could be??

In fact, by the end of the article, I’ll be seriously considering breaking out the local knowledge from the world-supplied knowledge.

A related topic arises in Bot. Here is its init:

class Bot:
    def __init__(self, x, y, direction=Direction.EAST):
        self.name = 'R'
        self.direction_change_chance = 0.2
        self._knowledge = Knowledge(Location(x, y), direction)
        self.state = Walking()
        self._old_location = None

Once we start storing things in _knowledge, we should start wondering whether we should store everything there. Why do we have any variable attributes in Bot at all?

Added in Post

The name and direction change chance never change after the Bot is created. The knowledge instance never changes, although its contents are updated on each turn. The state changes on each turn. The old location changes whenever we take a step, which is every turn right now but need not be.

When we find member variables that change at different rates, that is a clue that there might be a need for another object, or to move things around. The knowledge changes on every turn. Perhaps Bot should know the static things and all the variable things should be in Knowledge. I just noticed this in reviewing the article. You’ll see below that I have another reason to suspect that things aren’t quite right in Bot.

As things stand, the Bot’s state machine doesn’t make all the decisions. The Bot instance decides randomly to change direction, and the Bot always takes a step as its last action on each turn.

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

And that’s why _old_location is in Bot, because after we create our actions, we record any expectations that we want to check next time around:

    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 []

We have the record and check methods as places where we can record further things to check if and when we come up with more expectations about results.

Couldn’t all this be encoded into the state machine? I suppose it could be, but if it were, I think the state machine would be much more complex. The direction change and stepping occur no matter what state the current machine is in. So if it were all in a state machine, I think there would be many more states and/or substates. I don’t think we need that, at least not yet.

However, if we’re going to keep one kind of Bot-side info in knowledge, the energy, shouldn’t we keep the old location there as well? What about the name? What about the direction change chance?

Or should we buckle down and have two kinds of knowledge, world-provided and local, and keep them in two separate objects and pass both those objects to the state machine?

While I think it would be a bit much to have just the energy in a separate object, if there were two or three attributes in a bot-local information object, that might be OK.

That thought puts a new question into my mind: just what does Bot really do? One thing that it seems to do is to serve as a point of contact for tests, with all these methods:

    def has_block(self):
        return self._knowledge.has_block

    def can_take(self):
        return self._knowledge.can_take

    def can_drop(self):
        return self.vision.match_forward_and_one_side('_', 'B')

    def change_direction(self):
        return [self._knowledge.new_direction().name()]

    def forward_location(self):
        return self.location + self.direction

Those methods all really defer to _knowledge, since vision, location, and direction are all in _knowledge.

I remove can_take and can_drop after noticing that they are not really used.

Arrgh! I try to remove forward_location and a test fails. I think the test is wrong:

    def test_bot_cant_take_diagonally(self):
        world = World(10, 10)
        bot = DirectConnection(world).add_bot(5, 5)
        world.add_block(4, 4)
        world.add_block(6, 4)
        world.add_block(4, 6)
        world.add_block(6, 6)
        world.take_forward(bot)
        assert not bot.has_block()

I think we have a client-side bot in hand and that World is happy to try to use it. We need a world-bot here.

    def test_bot_cant_take_diagonally(self):
        world = World(10, 10)
        client_bot = DirectConnection(world).add_bot(5, 5)
        world_bot = world.map.at_id(client_bot.id)
        world.add_block(4, 4)
        world.add_block(6, 4)
        world.add_block(4, 6)
        world.add_block(6, 6)
        world.take_forward(world_bot)
        world.update_client_for_test(client_bot)
        assert not client_bot.has_block()

Still green but I want to be more certain. I’ll put a block where it can be taken and see if the test fails. When I add a block to the Bot’s EAST: world.add_block(6,5), the test fails. I am satisfied. Now I should be able to remove that method.

I’m out of spoons. Commit: Fix test, allows removal of Bot.take_forward.

Must sum up.

Summary

We started out thinking that we’d review some code and find things to refactor or otherwise improve. Here at the end of the session that has started to happen. In particular, we set out to answer the question of what Bot “really” does and we’re in the process of discovering that it has at least two responsibilities: It creates the action list for each turn, with the able assistance of the state machine. That’s a good thing. But it also serves as an inspection point for a lot of tests that ask it questions that would not otherwise concert the Bot, for example:

    def has(self, entity):
        return self._knowledge.has(entity)

    def receive(self, entity_id: int):
        self._knowledge.receive(entity_id)

    def remove(self, entity):
        self._knowledge.remove(entity)

    def do_something_only_for_tests(self, connection):
        actions = self.get_actions()
        self.perform_actions_only_for_tests(actions, connection)
        return actions

    def has_block(self):
        return self._knowledge.has_block

We’ve removed another three or four similar methods that were only needed by tests, and we’ve found and fixed a test that wasn’t testing quite what we thought it was. But those five methods above tell us that we have a problem.

What problem, you ask? Well, the simplest way of looking at it is that common wisdom is that an object should generally only have a single responsibility, and Bot’s primary responsibility, creating the actions list, seems like one that it should have. So this role as some kind of test intermediary is inappropriate. According to “common wisdom”. Also, according to my own experience, which is that when an object has more than one responsibility, very likely there are two or more objects that, if they existed, would serve better than the one.

So I am quite willing to consider Bot’s two-faced character as indicating that the code can be better, not because I read it in a book, but because I read it in a book and then have tried the idea so often that it is part of my own evaluation of how thing should be.

But I am out of spoons. I’ll have more spoons tomorrow, and we’ll continue to improve things.

See you then!