Concerns

The Repo on GitHub

We’ll look at the list of things to work on, but yesterday’s experience has sort of set me back on my heels.

My friend GeePaw Hill said to me, not long ago, that he had seen me write sloppy code, but had not seen me doing sloppy thinking. I took that as intended, and in fact was not minded to deny it then or now. However, I don’t want to write sloppy code, but sometimes I do. I like to think that my practice is such that when I inevitably do so, I find it and improve it.

What causes “sloppy code”, if not sloppy thinking? Well, certainly rushing will do it. Working when tired might do it, though I suppose that might make one’s thinking sloppy as well. As a program grows, if we do not keep up with refactoring, it will inevitably become sloppy, because when ideas are in the wrong places, the code to deal with things is forced to become sloppy. If an object isn’t helping us enough, new code inevitably becomes more convoluted, more sloppy.

Yesterday’s code improvement was good, even very good. We went from this:

class Bot:
    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

To this:

class Bot:
    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

Much better. We might consider one more refactoring to be an improvement, like this:

    def get_actions(self):
        actions = self.get_intended_actions()
        self.record_expectations(actions)
        return actions

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

I do see that as an improvement, according to my standards of code. I mentioned in yesterday’s article that there might be an additional improvement, but we were done for the day. Commit: Extract Method.

When I look at the original version above, I see sloppy code. I see if statements and odd variable setting going on, without discernible reason. I see code that clearly mixes creating actions with other notions. It’s—how can I put this—sloppy.

And that code didn’t get there in one go. That method, originally called do_something, has been around a long time, collecting small changes. I’ve been showing that method off and on since August. And yet, I’ve let it get that sloppy, right in front of you and everyone.

I don’t want to do that kind of thing. Now, I know that on any given day, I give you and this project what I’ve got, so I’m not here to beat myself up. I am, however, here to resolve to pay more attention to sloppy code, and to slow down and deal with it rather than rush on ahead, leaving a trail of sloppy code behind me.

We could probably browse a few classes and look for code that needs improvement. I’m sure it wouldn’t be hard to find some. But we do have things we want to accomplish. Here’s the current list:

• Prioritize error if can't move, because `_old_location` is weird.
• Provide and use an entry in DirectConnection to take a list of many actions;
• Refactor `execute` (I think I meant `get_actions` here, but we'll see.)
• Facilitate removal of methods like step and take from DirectConnection
• Continue to refine protocol between DC, Cohort, and Bot;
• Finish addition of bots
• Enhance Cohort to add bots;
• Use Cohort in game;
• Deal with errors, including avoiding exceptions and passing error info in the knowledge;
• Deal with fact that Location cannot be JSONized;
• Clean up execute in World, possible new object.
• [Possible] Move to better separation between Bot and World in tests.
• [Possible] Make it easier to create the little action dictionaries;

Let’s look at the DireectConnection issue.

class DirectConnection:
    def __init__(self, world):
        self.world = world

    def add_bot(self, x, y, direction=Direction.EAST):
        bot_id = self.world.add_bot(x, y, direction)
        result_dict = self.world.fetch(bot_id)
        client_bot = Bot(x, y)
        client_bot._knowledge.update(result_dict)
        return client_bot

    def step(self, cohort, client_bot_id):
        rq = [ {'entity': client_bot_id, 'verb': 'step'}]
        self.run_request(cohort, client_bot_id, rq)

    def take(self, cohort, client_bot_id):
        rq = [ {'entity': client_bot_id, 'verb': 'take'}]
        self.run_request(cohort, client_bot_id, rq)

    def drop(self, cohort, client_bot_id, holding_id):
        rq = [ {'entity': client_bot_id, 'verb': 'drop', 'holding': holding_id}]
        self.run_request(cohort, client_bot_id, rq)

    def run_request(self, cohort, client_bot_id, rq):
        results = self.world.execute(rq)
        cohort.update(results)

There are some issues here. The one we’re after is those three methods step, take, and drop. They are only used by tests, if I’m not mistaken, and they know more than the connection should know about things work. There is no reason for the connection to know the format of the messages it sends.

We’ll look for senders of these and see what we need to do. Worst case we can move the creation of those short requests over to the test side of things: that wouldn’t even be a bad thing to do. Short requests make sense for testing.

PyCharm says there are two references each to step and take and none to drop. We’ll remove it and see if anything breaks.

A test fails. Hmm, PyCharm. Anyway, this is the test, and it looks like it needs some work:

    def test_bot_drops_and_world_receives(self):
        world = World(10, 10)
        connection = DirectConnection(world)
        client_bot = connection.add_bot(5, 5)
        block_id = world.add_block(6, 5)
        block = world.map.at_xy(6, 5)
        assert isinstance(block, WorldEntity)
        cohort = Cohort(client_bot)
        connection.take(cohort, client_bot.id)
        assert client_bot.has_block()
        assert not world.map.at_xy(6, 5)
        connection.drop(cohort, client_bot.id, block_id)
        test_block = world.map.at_xy(6, 5)
        assert isinstance(test_block, WorldEntity)

PyCharm regains my favor by inlining that method call for me:

    def test_bot_drops_and_world_receives(self):
        world = World(10, 10)
        connection = DirectConnection(world)
        client_bot = connection.add_bot(5, 5)
        block_id = world.add_block(6, 5)
        block = world.map.at_xy(6, 5)
        assert isinstance(block, WorldEntity)
        cohort = Cohort(client_bot)
        connection.take(cohort, client_bot.id)
        assert client_bot.has_block()
        assert not world.map.at_xy(6, 5)
        rq = [{'entity': client_bot.id, 'verb': 'drop', 'holding': block_id}]
        connection.run_request(cohort, client_bot.id, rq)
        test_block = world.map.at_xy(6, 5)
        assert isinstance(test_block, WorldEntity)

I do like that. Now I can remove that method. Green. Commit: inline drop to tests and remove.

I wonder if I can select the method take in DirectConnection and inline it everywhere it’s used. Let’s try it.

I select inline and remove. It does. Green: inline take to tests and remove.

I don’t think I’ve ever tried inlining quite this way. I am pleased that PyCharm can do this. One more.

Done, easy-peasy, green. Commit: inline step to tests and remove.

I’m really glad that I tried that inlining. I was prepared to change those tests manually. It’s so much easier with a power tool, and it does a pretty reasonable job as well.

Let’s look at that test now: it’s probably a bit messy looking:

    def test_bot_drops_and_world_receives(self):
        world = World(10, 10)
        connection = DirectConnection(world)
        client_bot = connection.add_bot(5, 5)
        block_id = world.add_block(6, 5)
        block = world.map.at_xy(6, 5)
        assert isinstance(block, WorldEntity)
        cohort = Cohort(client_bot)
        rq1 = [{'entity': client_bot.id, 'verb': 'take'}]
        connection.run_request(cohort, client_bot.id, rq1)
        assert client_bot.has_block()
        assert not world.map.at_xy(6, 5)
        rq = [{'entity': client_bot.id, 'verb': 'drop', 'holding': block_id}]
        connection.run_request(cohort, client_bot.id, rq)
        test_block = world.map.at_xy(6, 5)
        assert isinstance(test_block, WorldEntity)

We could do a couple of extracts here. I don’t know if it’s clever enough to extract into local functions. It’s not, gives me this:

    def test_bot_drops_and_world_receives(self):
        world = World(10, 10)
        connection = DirectConnection(world)
        client_bot = connection.add_bot(5, 5)
        block_id = world.add_block(6, 5)
        block = world.map.at_xy(6, 5)
        assert isinstance(block, WorldEntity)
        cohort = Cohort(client_bot)
        self.do_take(cohort, connection, client_bot)
        assert client_bot.has_block()
        assert not world.map.at_xy(6, 5)
        self.do_drop(cohort, connection, client_bot, block_id)
        test_block = world.map.at_xy(6, 5)
        assert isinstance(test_block, WorldEntity)

    def do_drop(self, cohort, connection, client_bot, block_id):
        rq = [{'entity': client_bot.id, 'verb': 'drop', 'holding': block_id}]
        connection.run_request(cohort, client_bot.id, rq)

    def do_take(self, cohort, connection, client_bot):
        rq1 = [{'entity': client_bot.id, 'verb': 'take'}]
        connection.run_request(cohort, client_bot.id, rq1)

I might prefer those helpers as local def statements, but let’s let it ride this way.

There is at least one assert in there that was just there to assure me that I had the right kind of object in hand. Let’s see if we can simplify and rearrange this to be a bit more clear, as long as we’re looking at it.

    def test_bot_drops_and_world_receives(self):
        world = World(10, 10)
        connection = DirectConnection(world)
        client_bot = connection.add_bot(5, 5)
        cohort = Cohort(client_bot)

        block_id = world.add_block(6, 5)
        assert (block := world.map.at_xy(6, 5)) is not None

        self.do_take(cohort, connection, client_bot)
        assert client_bot.has_block()
        assert world.map.at_xy(6, 5) is None
        
        self.do_drop(cohort, connection, client_bot, block_id)
        assert world.map.at_xy(6, 5) is block

I find that a bit better. We could extract little methods for the three bits that are broken out, but I think this will do.

In production code, I would treat my desire to space something away from what’s next to it as a sign that an extract needs to be done. In tests, I prefer to keep the assertions in line when I can. Just a personal preference: if you wanted to extract little methods here I wouldn’t object. (I think I would object to spaced-out production code.)

Reflection

With almost everything automated, we’ve removed three test-only methods from DirectConnection, which is now quite short:

class DirectConnection:
    def __init__(self, world):
        self.world = world

    def add_bot(self, x, y, direction=Direction.EAST):
        bot_id = self.world.add_bot(x, y, direction)
        result_dict = self.world.fetch(bot_id)
        client_bot = Bot(x, y)
        client_bot._knowledge.update(result_dict)
        return client_bot

    def run_request(self, cohort, client_bot_id, rq):
        results = self.world.execute(rq)
        cohort.update(results)

We have items on our to-do list to deal with the add_bot:

• Continue to refine protocol between DC, Cohort, and Bot;
• Finish addition of bots
• Enhance Cohort to add bots;

It seems very likely that DC is going to evolve down to just a single method. That method represents a single exchange across the wire, a request from the client and a response from the server. That seems likely to be a good thing. It remains, someday, to actually write a connection that works over a socket. Someday. I’m in no rush.

We have accomplished what we set out to do and worked a bit to make the code more clear, even in a test. And we’ve discovered that PyCharm can inline between classes, at least sometimes. That’s sure to be useful in the future.

Summary

I think we’ve done about the right amount of work for this session. We removed three methods from DirectConnection, leaving two operational methods, one of which will probably be removed next time. In the course of doing that, we kept the tests running, and made use of PyCharm’s convenient ability to inline the specialized code from those methods.

Along the way, I spotted some things about Bot that need work. But that’s for next time. See you then!

Hey!

I just noticed in reviewing the article that run_request makes no use of the client_bot_id. Change Signature. Green Commit: remove unused parameter.

The world is a slightly better place. See you next time!