The Repo on GitHub

We’ve had good luck with take and drop. Let’s see if Map would like to help World with the step method. Some nice improvements … and a GitGlitch.

Doubtless, having called that shot, we’ll do something else entirely, but let’s at least take a look at the idea.

Our taking and dropping methods look like this:

    def take_forward(self, bot):
        take_location = bot.location + bot.direction
        is_block = lambda e: e.name == 'B'
        if block := self.map.take_conditionally_at(take_location, is_block):
            bot.receive(block)

    def drop_forward(self, bot, entity):
        drop_location = bot.location + bot.direction
        if self.map.place_at(entity, drop_location):
            bot.remove(entity)

And step is rather different:

    def step(self, bot):
        location = self.bots_next_location(bot)
        self.map.attempt_move(bot.id, location)  # changes world version
        real_bot = self.map.at_id(bot.id)
        self.set_bot_vision(real_bot)
        self.set_bot_scent(real_bot)

I don’t find this to be lovely. The fiddling with getting the “real bot” from map shouldn’t be necessary any more, if our client-server connection is correctly separating things. And if it were to have a name world_bot would be a better one.

Do the tests run if we don’t do that?

    def step(self, bot):
        location = self.bots_next_location(bot)
        self.map.attempt_move(bot.id, location)  # changes world version
        self.set_bot_vision(bot)
        self.set_bot_scent(bot)

One test fails. What is it?

    def test_we_get_scent(self):
        world = World(10, 10)
        block = Block(5, 5)
        world.add(block)
        client_bot = world.add_bot(4, 6)
        world.step(client_bot)
        world.update_client_for_test(client_bot)
        assert client_bot._knowledge._scent == 3

I suspect that goofy update for test thing. Let’s get the world bot and check scent there first, just to be sure nothing truly odd has happened.

    def test_we_get_scent(self):
        world = World(10, 10)
        block = Block(5, 5)
        world.add(block)
        client_bot = world.add_bot(4, 6)
        world_bot = world.map.at_id(client_bot.id)
        world.step(client_bot)
        world.update_client_for_test(client_bot)
        assert world_bot._knowledge._scent == 3
        assert client_bot._knowledge._scent == 3

Sadly, this fails on the world assertion:

>       assert world_bot._knowledge._scent == 3
E       assert 0 == 3

This version of the test, however, passes:

    def test_we_get_scent(self):
        world = World(10, 10)
        block = Block(5, 5)
        world.add(block)
        client_bot = world.add_bot(4, 6)
        world_bot = world.map.at_id(client_bot.id)
        world.step(world_bot)  # stepping world bot, not client bot
        world.update_client_for_test(client_bot)
        assert world_bot._knowledge._scent == 3
        assert client_bot._knowledge._scent == 3

I think this is more nearly right. What would happen if, instead of telling the world to step, we told the client bot to do something?

    def test_we_get_scent(self):
        world = World(10, 10)
        block = Block(5, 5)
        world.add(block)
        client_bot = world.add_bot(4, 6)
        world_bot = world.map.at_id(client_bot.id)
        client_bot.do_something()
        assert world_bot._knowledge._scent == 3
        assert client_bot._knowledge._scent == 3

The test is green. Commit: simplify world.step, fix old-style test.

Yikes. I have a weird thing in Git/GitHub. I forgot to change the commit message, so I tried to amend. GitHub rejected that and seemed to be telling me to pull, which was also rejected.

Gumption trap. Excuse me while I try to work out what to do here.

I’m making things worse. I think I just pushed a branch to GitHub. One of these days I really need to learn this tool, instead of just trying to stay within the few commands I know.

origin detached head

OK, I think I have it back to normal mode, though I honestly do not know this tool as well as I’d like to. I just try to stick to the straight and narrow, but I got a wheel off the pavement here and things got nasty.

Let’s get back to step, which currently looks like this:

    def step(self, bot):
        location = self.bots_next_location(bot)
        self.map.attempt_move(bot.id, location)  # changes world version
        real_bot = self.map.at_id(bot.id)
        self.set_bot_vision(real_bot)
        self.set_bot_scent(real_bot)

That looks familiar but not as much like what I used to have. I think I lost some changes along the way. No matter:

    def step(self, bot):
        location = self.bots_next_location(bot)
        self.map.attempt_move(bot.id, location)  # changes world version
        self.set_bot_vision(bot)
        self.set_bot_scent(bot)

And the test fails again. I must have somehow managed to get checked out without the stuff I mistakenly branched. Makes sense. Fix the test:

    def test_we_get_scent(self):
        world = World(10, 10)
        block = Block(5, 5)
        world.add(block)  # comment?
        client_bot = world.add_bot(4, 6)
        client_bot.do_something()
        assert client_bot._knowledge._scent == 3

Passing. Let’s see if we can commit: simplify world.step, fix old-style test.

Yay, it worked. Now let’s see whether we like how the step works, compared to the other two methods:

class World:
    def step(self, bot):
        location = self.bots_next_location(bot)
        self.map.attempt_move(bot.id, location)  # changes world version
        self.set_bot_vision(bot)
        self.set_bot_scent(bot)

    def bots_next_location(self, bot):
        location = bot.location + bot.direction
        return bot.location if self.is_off_world(location) else location

    def take_forward(self, bot):
        take_location = bot.location + bot.direction
        is_block = lambda e: e.name == 'B'
        if block := self.map.take_conditionally_at(take_location, is_block):
            bot.receive(block)

    def drop_forward(self, bot, entity):
        drop_location = bot.location + bot.direction
        if self.map.place_at(entity, drop_location):
            bot.remove(entity)

I’m slightly concerned by the fact that the attempt method changes the location but the other two also make changes, so I think that’s OK.

We’re also doing that bots_next_location which may or may not return the cell in front of us. So let’s look at the attempt method:

class Map:
    def attempt_move(self, id, location: Location):
        entity = self.contents[id]
        if self.location_is_open(location):
            entity.location = location

    def location_is_open(self, location: Location):
        return self.is_within_map(location) and not self.is_occupied(location)

Let’s change the step to be like the other two … and then do something about the duplication.

class World:
    def step(self, bot):
        new_location = bot.location + bot.direction
        self.map.attempt_move(bot.id, new_location)  # changes world version
        self.set_bot_vision(bot)
        self.set_bot_scent(bot)

Green. Commit: make step look more like take and drop.

Now when we look at those methods, we see some “feature envy” in that first line that calculates the location in front of the bot. We might argue that we should ask the bot for its forward location. Let’s do that:

class World:
    def step(self, bot: Bot):
        new_location = bot.forward_location()
        self.map.attempt_move(bot.id, new_location)  # changes world version
        self.set_bot_vision(bot)
        self.set_bot_scent(bot)

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

Green. Commit use and create bot.forward_location().

I discover an intermittent test, the scent one. I suspected that, and ran it many times without failure. But it can change direction. We’ll fix that in a moment. Fist, though, we’ll use our new method in taking and dropping, and we’ll do some inlining while we’re at it.

class World:
    def step(self, bot: Bot):
        self.map.attempt_move(bot.id, bot.forward_location())  # changes world version
        self.set_bot_vision(bot)
        self.set_bot_scent(bot)

    def take_forward(self, bot):
        is_block = lambda e: e.name == 'B'
        if block := self.map.take_conditionally_at(bot.forward_location(), is_block):
            bot.receive(block)

    def drop_forward(self, bot, entity):
        if self.map.place_at(entity, bot.forward_location()):
            bot.remove(entity)

And the test fix:

    def test_we_get_scent(self):
        world = World(10, 10)
        block = Block(5, 5)
        world.add(block)  # comment?
        client_bot = world.add_bot(4, 6)
        client_bot.direction_change_chance = 0.0  # <===
        client_bot.do_something()
        assert client_bot._knowledge._scent == 3

Green. Commit: refactoring step take and drop. fixed intermittent test.

Now we have some unused methods:

class World:
    def bots_next_location(self, bot):
        location = bot.location + bot.direction
        return bot.location if self.is_off_world(location) else location

    def is_off_world(self, location):
        return self.clip(location.x, self.width) != location.x \
            or self.clip(location.y, self.height) != location.y

    def clip(self, coord, limit):
        return 0 if coord < 0 else (limit if coord > limit else coord)

Commit: remove unused methods.

There are two methods in here that are test-only:

class World:
    def is_empty(self, drop_location):
        return not self.map.at_xy(drop_location.x, drop_location.y)

    def update_client_for_test(self, client_bot):
        result_dict = self.fetch(client_bot.id)
        client_bot._knowledge.update(result_dict)

I’ve made a note of those. I’d like to revise the tests to get ride of them. For now, we’ve done what we set out to do. Let’s sum up:

Summary

Just a little refactoring exercise, with a brief codus interruptus by a Git Glitch. Kind of derails the train of thought, but I managed to fumble my way to sorting it out. What did I learn? Mostly I learned “don’t try to amend a commit”.

The changes to step weren’t much, but they led us to a method on Bot that avoided six (6) violations of Demeter, and, with some inlining, saved three lines of code. (Mixed blessing there, the explaining variable name might make for easier reading at least for some readers. Horses for courses, you get to have your own coding standards, and so do I.)

The World class is starting to look like the kind of OO code that I like to see, with very short methods that just do one thing.

Along the way we found and improved one of our too-sensitive too-invasive tests. There are a lot of those, tests that tell a little story. I like that they tell a story, and I don’t like the fact that they are all integration-style tests, exercising two or more classes. I don’t like the fact that they are fragile, since they know too much about the specifics of methods instead of being able to just do a thing and check a result.

But I’m not going to spend specially allocated time improving them just because I think I should have done better. Well, I’m probably not going to do that. What I will do is try to improve them if and when they trouble me, as we did this morning with that one scent test. It failed because of its own weakness, and we made it pass by making it less weak.

All in all, a pleasant morning with a net reduction in code, and, by my lights, an increase in clarity and design quality. Just what I needed.

See you next time!