The Repo on GitHub

I’m still slightly troubled by the messages and used_ids attributes of World. Is Method Object our solution? Or is this already a Method Object? Hey! This stuff actually works!

This is the entry point to a request to World:

    def execute_requests(self, actions_list):
        self.ids_used = set()
        self.messages = []
        valid_actions = self.get_valid_list(actions_list)
        self.execute_actions(valid_actions)
        updates = [self.fetch(bot_id) for bot_id in self.ids_used]
        return { 'updates': updates, 'messages': self.messages }

I am troubled by this code, for two very small reasons. First, The pattern formed by get_valid_list and the fetch for updates. Those two create local variables. The call in the middle, to execute_actions does not. Consider this pattern:

x = self.get_x()
y = self.get_y()
z = self.get_z()
return 'something involving all those actions'

I think that the above is “better” than this:

x = self.get_x()
self.mumble()
z = self.get_z()
return 'something involving all those actions'

I freely grant that that’s just a vague feeling, but after six decades of programming or so, I am inclined to pay attention to such feelings.

My second concern in that same method is the initialization of the two member attributes, ids_used and messages. When we see that sort of thing, it’s clear that those attributes are changing at a different rate from other attributes of the object, and that’s a sign that perhaps they belong in a separate object. One common way of dealing with that situation is the pattern Method Object.

In essence, the Method Object pattern suggests creating a new object in this method, and deferring all the work to that object. That one can legitimately hold whatever member variables are useful to it, and we avoid passing parameters down into the called methods.

We could do that here, and it would be a fun exercise. Is it needed? Is it appropriate?

Let’s look at World’s init. Having already copied it for pasting below, I see that it is interesting:

class World:
    def __init__(self, max_x, max_y):
        self.width = max_x
        self.height = max_y
        self.map = Map(max_x, max_y)
        self.ids_used = set()
        self.messages = []

Do we actually use width and height? I believe not. Remove them. Green. Commit: remove unused width and height.

class World:
    def __init__(self, max_x, max_y):
        self.map = Map(max_x, max_y)
        self.ids_used = set()
        self.messages = []

Now World has just our two members that change on each call to execute_requests, and the map. The map changes on every request. Not the instance, it remains the same, but its contents change as Bots move around. We could imagine the other two attributes as remaining the same instance, being changed on each cycle, emptied at the beginning and used to compute the result at the end.

I think we can conclude that the World is, in effect, a method object for execute_requests. If that is an accurate portrayal, we would expect no methods in World that are not part of executing requests.

Here are all the attributes of World:

'add_bot_message', '_add_entity', '_add_message', 'add_block', 'add_bot', 'add_bot_action', 'assign_parameters', 'drop_forward', 'drop_forward_action', 'entity_from_id', 'execute_action', 'execute_actions', 'execute_requests', 'fetch', 'get_valid_list', 'ids_used', 'is_empty', 'map', 'messages', 'set_bot_scent', 'set_bot_vision', 'step_action', 'take_forward_action', 'turn_action']

By golly, they all are part of executing requests with one exception, is_empty, which is used only by tests. We’ll allow it as a useful exception. I note that my use of the private underscore signal is still erratic. I need to work on that.

So … World is essentially an object for processing requests. It holds a Map, which is the only memory that really persists between requests.

Let’s think about that.

Arguably a better structure for all this would be to make the Map be the top dog, and to have a connection from the client to the Map, and when a request comes through, the Map would create a World (probably renamed to something mundane(!) like RequestProcessor), pass it itself plus the requests, receive the results, pass them through the connection, dropping the World instance.

Aside from the name change, I think the World class would be exactly the same as it is now. We could remove the initialization of used_ids and messages in execute_requests, since they’d be initialized in the __init__. Otherwise, same same.

Should we do it, then?

I’m going to say no. The improvement would be essentially invisible, and we would have to rewire a bunch of tests, because presently creating the World creates the Map, not vice versa. (We could hack our way around that but I think that hack would be ugly enough to eliminate the value of the change.)

What about that asymmetry with the temp variables?

I think we’ll let that be, at least for now. The request execution updates the map. It seems to have no natural object or list to return. It does update both used_ids and messages, which is why they are member variables in the Method Object that is World. Maybe something will happen that makes the asymmetry more troubling, in which case we’ll see how to fix it. If not, it’s not the worst thing I’ve ever seen.

Summary

After this reflection, I actually feel pretty good about what we have here. The World object, after all this time, needs just three member variables. It handles requests to move bots and other entities around, and it defers most of the decisions about that to the Map. The division of responsibilities seems quite sensible, without any notable weird hookups or workarounds.

We’ve been evolving this program since August. Here we are in December and our World design is alive, just about where our latest understanding would put it. This is a good thing. It’s a tribute, not to my own genius (LOL) but to the effect of trying always to focus on small scale improvements, moving toward our best understanding of good design.

In short: this stuff actually works!