A Better Way??

The Repo on GitHub

I have an idea for that Knowledge class. Let’s see if it Just Might Work.`

We noticed yesterday that the Knowledge class has some issues. We mentioned the large number of getters and setters, the use of properties for some ideas that might better be functions of no arguments, and the fact that methods seemed to me intertwined and intermingled with the getters and setters. And, the big issue, there are three long methods that all independently know the keys of the object, that need to be kept in sync, and that there is nothing keeping them in sync.

Here’s the class:

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._held_entity = None
        self._id = None
        self._location = location
        self._scent = 0
        self._vision = Vision([], self.location, self.direction)
        # local Bot client-side info
        self._energy = self.energy_threshold

    def as_dictionary(self):
        held = 0 if not self._held_entity else self._held_entity.id
        return {'direction': self._direction,
                'held_entity': held,
                'id': self._id,
                'location': self._location,
                'scent': self._scent,
                'vision': self._vision}

    def update(self, update_dictionary):
        self.direction = update_dictionary['direction']
        self.receive(update_dictionary['held_entity'])
        self.id = update_dictionary['eid']
        self.location = update_dictionary['location']
        self._scent = update_dictionary['scent']
        self.vision = update_dictionary['vision']

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

    @property
    def id(self):
        return self._id

    @id.setter
    def id(self, id):
        self._id = id

    def has_energy(self):
        return self._energy >= self.energy_threshold

    def use_energy(self):
        self._energy = 0

    def gain_energy(self):
        self._energy += 1

    @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 direction(self):
        return self._direction

    @direction.setter
    def direction(self, direction):
        self._direction = direction

    @property
    def location(self):
        return self._location

    @location.setter
    def location(self, location):
        self._location = location

    def has(self, entity):
        return entity == self._held_entity

    @property
    def has_block(self):
        return self._held_entity  # and self._held_entity.name == 'B'

    @property
    def can_take(self):
        is_scent_ok = self._scent <= self.take_threshold
        return is_scent_ok and self.vision.match_forward_and_one_side('B', '_')

    @property
    def can_drop(self):
        vision_ok = self.vision.match_forward_and_one_side('_', 'B')
        scent_ok = self._scent >= self.drop_threshold
        return vision_ok and scent_ok

    def receive(self, entity):
        self._held_entity = entity

    def remove(self, entity):
        if self._held_entity == entity:
            self._held_entity = None

I have a rough starting idea for an object that might be better than what we have here. We know something now that we didn’t know when we started with Knowledge: it will be passed back and forth between server and client as a dictionary, a collection of keys and values. That suggests that it should perhaps use a dictionary as its internal structure.

Or at least most of its internal structure. We notice that there is currently one element of Knowledge that is not shared between server and client, the _energy value. That’s used in our client-side state machine to decide when we have walked off the ennui that overtakes the Bot after it picks up or drops something. So, perhaps, Knowledge is a class that has a dictionary of shared information, plus member variables that are pertinent to the client Bot and not shared with the server.

Aside

Arguably, we could imagine that the Bot object might hold things pertinent to the client only, plus an instance of Knowledge, which might even be named SharedKnowledge. Just now, I think we’d like not to do that, because the state machine works on Knowledge and expects to find energy in there. So if we were to decide to move it to Bot we should do it either before, or after we improve Knowledge. I propose “after”, mostly because improving Knowledge seems more valuable and besides, it’s what I thought I’d work on today.

My thought coming into this was that we might TDD up a new kind of Knowledge, and then plug it in. Another possibility would be to refactor this one to be more like we’d like. The “obvious” way is to change it all in one go, which probably wouldn’t take that long, but might go an hour or so between commits. I wonder if there is a way to do it incrementally. That would be good practice, and a good practice as well.

But first I want to try my basic idea, so let me describe it a bit:

Knowledge Idea

The new / revised Knowledge will contain a dictionary, holding the shared information that passes back and forth between client and server. It will include a special method, maybe required_keys, listing all the keys in the shared bits. The method as_dictionary will basically just return the saved dictionary, and update will copy the input dictionary’s values into ours.

Perhaps we should just use it, as it will have been created by our end of the collection, reconstituted from the JSON or whatever goes across the wires. We do have at least one special bit of handling, which is to turn the vision that comes across into an instance of the client’s Vision object. Today, that’s done with a specialized setter property, like this:

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

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

I think we might try just using it, which would make both update and as_dictionary trivial.

I have something clever in mind for the setters and getters. First, we should check to see which ones we actually use, and remove the others. But then, assuming the bulk of them remain, or at least the bulk of the getters, I was thinking we might override __getattr__ (or maybe __getattribute__, the mere existence of which tells us that this might be too deep in the bag of tricks). With that override in place, we would translate a call like knowledge.location into self.knowledge_dict['location'], allowing class users to continue to behave as if the class had ordinary attributes.

We will definitely want to TDD at least an experimental class to try that idea out. It may not be too clever to live, but it is surely too clever to be done off the cuff.

Let’s get to it:

Exploration

First let’s see about users of all these getters and setters.

I begin by moving all the actual methods down below the properties. Green, commit: move active methods after accessors.

A quick glance at the general properties tells me that the Bot class recognizes all the methods like id and direction and such, and forwards them all to its knowledge member. How are these used, I wonder?

As a check, I temporarily remove the Bot.id setter. One test fails, giving Bots some ids:

    def test_bots_dont_smell(self):
        map = Map(10, 10)
        for x in range(11):
            for y in range(11):
                block = Bot(x, y)
                block.id = 11*x + y
                map.place(block)
        scent = map.scent_at(Location(5, 5), 0)
        assert scent ==  0

OK, since this is testing Map, it is a world-side test, not a client side test. Pay no attention to the fact that I called the Bots block.

Let’s see what Map actually does, so we can get a clue what we’re testing. Presumably we just can’t smell Bots.

class Map:
    def relative_scent(self, location, current, desired_aroma):
        found = self.at_xy(current.x, current.y)
        scent = 0
        if found and found.name == 'B' and found.aroma == desired_aroma:
            scent = 4 - location.distance(current)
            if scent < 0:
                scent = 0
        return scent

I wonder what found could be. Clearly it could be None. Or it could be a MapEntity. We have no business putting an actual Bot into the map, as this test does.

If I do this:

class Map:
    def place(self, entity):
        assert isinstance(entity, WorldEntity)
        self.contents[entity.id] = entity

Five tests fail. (I started with MapEntity in there, and 38 failed. We’ll see MapEntity below, I think.)

Reflection

Suddenly we find ourselves two or three levels away from where we thought we would be working. I think we’ll skip unwinding the properties for now, simply replicating them. But let’s do go after these five failing tests. They’ll be doing things they shouldn’t be doing, like the scent one, which is probably one of the five.

In fact it is.

    def test_bots_dont_smell(self):
        map = Map(10, 10)
        for x in range(11):
            for y in range(11):
                block = Bot(x, y)
                block.id = 11*x + y
                map.place(block)
        scent = map.scent_at(Location(5, 5), 0)
        assert scent ==  0

Let’s do this:

    def test_bots_dont_smell(self):
        world = World(10, 10)
        world.add_bot(5, 5)
        bot_entity = world.map.at_xy(5, 5)
        bot_entity.aroma = 5
        map = world.map
        scent = map.scent_at(Location(5, 5), 5)
        assert scent ==  0

We make a smelly bot and show that the scent is not detected. More direct, more better. Can’t commit, we have broken tests, unless we comment out that type checking. We’ll do that and commit: test no longer stores bad things in the map.

The remaining failing tests are all testing the Map itself, using weird objects and the map_is_OK function:

    def test_map_with_block(self):
        world_map = Map(10, 10)
        block = Block(3, 4)
        world_map.place(block)
        other_map = FakeMap()
        map_entry = MapEntity(3, 4, 'B')
        other_map.contents.append(map_entry)
        assert world_map.map_is_OK(other_map)

    def test_map_successful_move(self):
        map = Map(10, 10)
        bot = Bot(5, 5)
        map.place(bot)
        map.attempt_move(bot.id, Location(6, 6))
        assert bot.location == Location(6, 6)

    def test_map_unsuccessful_move(self):
        map = Map(10, 10)
        bot = Bot(5, 5)
        map.place(bot)
        map.attempt_move(bot.id, Location(10, 11))
        assert bot.location == Location(5, 5)

    def test_map_rejects_bot_location(self):
        map = Map(10, 10)
        block = Block(5, 5)
        map.place(block)
        result = map.location_is_open(Location(5, 5))
        assert result is False

The WorldEntity class, which is what we are supposed to put into the Map, has bot and block class methods. I think we should be able to change the last three tests easily.

    def test_map_successful_move(self):
        map = Map(10, 10)
        bot = WorldEntity.bot(5, 5, Direction.EAST)
        map.place(bot)
        map.attempt_move(bot.id, Location(6, 6))
        assert bot.location == Location(6, 6)

    def test_map_unsuccessful_move(self):
        map = Map(10, 10)
        bot = WorldEntity.bot(5, 5, Direction.EAST)
        map.place(bot)
        map.attempt_move(bot.id, Location(10, 11))
        assert bot.location == Location(5, 5)

    def test_map_rejects_bot_location(self):
        map = Map(10, 10)
        block = WorldEntity.block(5, 5)
        map.place(block)
        result = map.location_is_open(Location(5, 5))
        assert result is False

Those three run as expected. The first one … changed like the others, passes:

    def test_map_with_block(self):
        world_map = Map(10, 10)
        block = WorldEntity.block(3, 4)
        world_map.place(block)
        other_map = FakeMap()
        map_entry = MapEntity(3, 4, 'B')
        other_map.contents.append(map_entry)
        assert world_map.map_is_OK(other_map)

But what is it trying to test? What is map_is_ok?

class Map:
    def map_is_OK(self, other: [MapEntity]):
        other_entities = other.get_entities()
        if not self.check_all_other_entities_are_valid(other_entities):
            return False
        return len(self.contents) == len(other_entities)

    def check_all_other_entities_are_valid(self, other_contents: [MapEntity]):
        other_entities_are_all_valid = True
        for map_entity in other_contents:
            if self.at_xy(map_entity.x, map_entity.y).name != map_entity.name:
                other_entities_are_all_valid = False
        return other_entities_are_all_valid

It seems to be providing a separate list of things that should be in the map and checking them, using the MapEntity class to hold the objects to be checked. Let’s rename the MapEntity class to MapTestEntity, in hopes of never being confused again.

Green. Commit: modify tests to use correct WorldEntity objects in Map.

Wow. 345 lines of article, where most lines are more than one line long and not a single bit of work on what we came in here to do.

Remember that I started this short sub-series with ‘Kondoizing’, yesterday, where we look around and find things that do not spark joy. When we’re in that mode, we shouldn’t be surprised to find things that lead us away from what we thought was most important. Now, certainly we seriously consider what’s most important, and these tests were clearly way out of whack, using client-side objects in world-side collections and tests. There are probably more of those to find, and I’d like to clean them all up.

I’d also like to address this issue with the Knowledge class. Let’s at least test the idea that I had for making a dictionary look like a class with members.

Eine Kleine TDDmusik

I want an object that contains a dictionary and makes the keys of the dictionary behave like regular members with getters and setters. We’ll start with a trivial and not very fun way of doing that, and then pull out some tricks from deeper in the bag.

I start with a bit of research, finding this on stack overflow, which isn’t quite what we need because it’s building a n object that is really a dictionary subclass. But it shows what we need to do, I think.

class TestDictionaryIdea:
    def test_hookup(self):
        assert False

I should make a snippet or whatever they call it. Test fails. Now write a real one:

class TestDictionaryIdea:
    def test_dict_with_dot_access(self):
        knowledge = NewKnowledge()
        knowledge.id = 100
        assert knowledge._dict["id"] == 100

And the starter class, which fails the test:

class NewKnowledge:
    def __init__(self):
        self._dict = dict()

So now we need to implement the set and get.

… after quite some delay …

OK, enough. This is harder than I thought. I have this test:

    def test_dict_with_dot_access(self):
        knowledge = NewKnowledge()
        knowledge.id = 100
        assert knowledge._dict["id"] == 100
        assert knowledge.id == 100
        knowledge.scent = 31
        assert knowledge._dict["scent"] == 31
        assert knowledge.scent == 31
        with pytest.raises(AttributeError):
            knowledge.unknown = 15

The idea here is that ‘id’ and ‘scent’ are legal key values in the NewKnowledge, and ‘unknown’ is not. So setting and getting the first two should work, and setting ‘unknown’ should raise the exception. I have the ‘id’ and ‘scent’ working, storing into the dictionary as intended, but everything I try for checking the keys either fails or causes an infinite recursion. I clearly need to learn more about setting and getting attributes … and possibly this idea is just a bit too deep in the bag.

No … I tried one more thing and this code is passing, We get to go out on a small win. Here’s the code and the final test with one more assertion.

class NewKnowledge:
    def __init__(self):
        self._dict = dict()

    def __getattr__(self, item):
        return self._dict[item]

    def __setattr__(self, key, value):
        known = ['id', 'scent']
        if key == '_dict':
            return super().__setattr__(key, value)
        if key in known:
            self._dict[key] = value
        else:
            raise AttributeError(f'{key} not in {known}')


class TestDictionaryIdea:
    def test_dict_with_dot_access(self):
        knowledge = NewKnowledge()
        knowledge.id = 100
        assert knowledge._dict["id"] == 100
        assert knowledge.id == 100
        knowledge.scent = 31
        assert knowledge._dict["scent"] == 31
        assert knowledge.scent == 31
        with pytest.raises(KeyError):
            _ = knowledge.unknown
        with pytest.raises(AttributeError):
            knowledge.unknown = 15

Obviously, the __setattr__ is the too-clever bit. It contains a list of the known keys, and the good new is, it’s the only such list. (We could perhaps make that a class variable but … there are issues.) __setattr_ checks to see if we are trying to assign to _dict and if we are, allows it. Then it checks for known keys and allows those to look in the _dict and raises AttributeError otherwise.

On the get side, we just fetch and if the dictionary does not contain the key, it will raise the KeyError.

One might ask: why does self._dict not go through _getattr__ and thus not work? The answer is more arcane than I can fully explain, but what I think I know is that __getattribute__ is called before calling __getattr__, and it finds the _dict.

So where are we? I would rate this idea as “promising but rather deep in the bag of tricks”. At this moment, I want to learn more, gain more comfort with things like this, so as to better decide whether this is OK or not. I’d really like it to work, because it would simplify knowledge a lot. We’ll see.

Summary

Very interesting morning (and early afternoon: I slept in late today and started quite late.) Starting right off looking at the Knowledge class, I noticed a test that was using a client-side Bot in a world-side test. Fixing that led me to check to see if other tests were misusing the Map and five were found. They were fairly quickly fixed.

Then I spent a big chunk of time trying to make my “simple” dictionary key idea work, with varying success. I had just about given up, was writing my summary of that, and got one more idea, tried it, and it worked.

So we have it working by the skin of our teeth, which isn’t much of a recommendation, but the exercise has provided some learning. I want to learn more but I suspect we’ll go further with the idea. For now, enough is enough.

See you next time!