Add Lightness

The Repo on GitHub

We are told that Colin Chapman used to say “Simplify, then add lightness”. Can we do that today?

Let’s look for something to improve. Here’s a sequence that I think is dubious:

class World:
    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)

    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

Why “dubious”?

What are the signs and portents that make me question that code? Well, the name bots_next_location seems odd, since we don’t always get there and the method returns one of two values, and then we check and attempt a move, which is surely checking again. So, curiosity piqued, we look further.

bots_next_location is used in two other places:

class World:
    def take_forward(self, bot: Bot):
        take_location = self.bots_next_location(bot)
        if take_location == bot.location:
            return
        entity = self.map.at_xy(take_location.x, take_location.y)
        if self.can_take_entity(entity):
            self.map.remove(entity.id)
            bot.receive(entity)

    def drop_forward(self, bot, entity):
        drop_location = self.bots_next_location(bot)
        if drop_location != bot.location and self.is_empty(drop_location):
            entity.location = drop_location
            bot.remove(entity)

Also questionable

This code may look pretty reasonable at first glance. But is bots_next_location really the right name? Would location_in_front or something of that nature be better?

And why are we doing all this location comparison stuff here in World? Shouldn’t that be deferred to the map or somewhere like that? And don’t take and drop really want to know something more specific and less detailed, like “is there something in front of the bot that can be taken” or “is there an empty cell in front of the bot”?

If the location in question is off-world, should we, in the take and drop cases. return a spurious on-world location, or should we return a None, indicating that there is no location? And, again, shouldn’t that detail be hidden further down?

What we have here is often called “feature envy”, or we might say “these objects aren’t helping us”. When these ideas are in play, we get code that looks more procedural than object-oriented. And, perhaps unfortunately, most of us are quite accustomed to code that looks like that and we let it go. But we can do better. Let’s do better.

Curiouser and curiouser. Let’s see what Map knows.

class Map:
    def __init__(self, width, height):
        self.width = width
        self.height = height
        self.contents = {}

OK, that’s enough to tell us that the Map can surely know whether a location is on the map, and let’s get serious here for a moment, it is the only object that knows where everything is, so it surely knows whether a cell has anything on it or not. It already has these useful-looking methods:

class Map:
    def location_is_valid(self, location: Location) -> bool:
        if self.is_occupied(location):
            return False
        return self.is_within_map(location)

    def is_within_map(self, location):
        return 0 <= location.x <= self.width and 0 <= location.y <= self.height

    def is_occupied(self, location):
        return self.at_xy(location.x, location.y)

We begin to see possibilities

We could certainly use is_occupied in drop_forward, which goes like this:

class World:
    def drop_forward(self, bot, entity):
        drop_location = self.bots_next_location(bot)
        if drop_location != bot.location and self.is_empty(drop_location):
            entity.location = drop_location
            bot.remove(entity)

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

More possibilities

We begin to see more possibilities, once we start to consider that this code could actually be improved.

Think just a bit deeper

Now there are really two things going on here, the boundary checking and the question of whether there is an empty cell at the intended location. What we know in the World, with this bot in hand (there’s a clue there) is the location of that bot and its direction. We do need to know whether to drop, and if we can, where.

A decision forms

I think we want to move away from the World validating Locations. That seems like a job for the map, which already includes some code for doing that job. Let’s write this out a bit longhand and then refactor.

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

And in Map:

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

When we look at is_occupied, we see something interesting:

class Map:
    def is_occupied(self, location):
        return self.at_xy(location.x, location.y)

This method could be called occupant, or occupant_or_none, or occupant_if_any. Or, given what seems to be the convention in Map, same names except ending in at.

Who’s using is_occupied? Just these two:

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

    def location_is_valid(self, location: Location) -> bool:
        if self.is_occupied(location):
            return False
        return self.is_within_map(location)

Duplication!

Wait just a second here, bunkie! Isn’t location_is_valid exactly the same as location_is_open? I think so. Find the users of the valid version and change them. Green. Commit: merge methods, remove unused.

Sometimes duplication isn’t line for line, but it’s duplication nonetheless.

Let’s check the drop_forward one more time and then look at the take.

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

Well, we’re still messing about with other people’s concerns here. When we set the entity location, we are relying on the Map to pick up on the fact that the entity has just shown up at this new location. This works, just barely, because the Map has no special structures for location, it just searches when it wants to find things by location. We really want the map to try to move the entity and let us know if it could: Like this:

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

And

class Map:
    def place_at(self, entity, drop_location):
        if self.location_is_open(drop_location):
            entity.location = drop_location
            self.place(entity)
            return True
        else:
            return False

    def place(self, entity):
        self.contents[entity.id] = entity

Note that I decided to do the place operation, just on the off chance that the entity was new to us. The place method was pre-existing, not created for this occasion.

Reflection.

Let’s review what happened here. I think it’s important, not for the immediate effect, but for the effect of always working this way.

We started with this:

class World:
    def drop_forward(self, bot, entity):
        drop_location = self.bots_next_location(bot)
        if drop_location != bot.location and self.is_empty(drop_location):
            entity.location = drop_location
            bot.remove(entity)

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

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

We currently have this:

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

I would argue that that’s simpler, more clear, and puts the responsibility of maintaining map integrity where it belongs, in Map. I would argue that we will always benefit from writing code like this, exactly because we can understand it more rapidly and more reliably, and because we are far more likely to have the subordinate objects correct if they manage their own affairs.

I would prefer that I write code that looks like this all the time. And in this program, I have not done as well as I would like. Yet. We’re not done with this program, and we can make it better every time we look at it.

You get to decide for yourself what kind of code is simplest for you to understand, clearest to you, and most likely to maintain integrity. I hope you’ll at least build up enough familiarity with the style reflected here so that you can make a legitimate decision rather than an habitual one, but we all get to sin our own sins.

But wasn’t this just a trade-off?

We’ll stop shortly, but let’s first review the Map code, to see whether we just traded one bad thing for another. It will turn out that we haven’t. Complexity has actually decreased.

Here are all the changes to Map:

class Map:
    def place_at(self, entity, drop_location):
        if self.location_is_open(drop_location):
            entity.location = drop_location
            self.place(entity)
            return True
        else:
            return False

    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)

Those last two methods used to look like this:

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

    def location_is_valid(self, location: Location) -> bool:
        if self.is_occupied(location):
            return False
        return self.is_within_map(location)

I think I’d argue that we added a useful capability to Map and otherwise simplified it as well. It’s not that we moved complexity from one place to another.

We put responsibility where it belonged, and the result was a net reduction in complexity.

In Chapman terms, we simplified one method, and in the process, added lightness.

How about that? Pretty nifty, if you ask me. Go thou and do likewise. See you next time!