The Repo on GitHub

We’ll take a look at the NewKnowledge prototype, but I think I’m going to go with it. No one here to talk me out of it, and I think the result will be much less cluttered. TL;DR: Not worth it. Fun, though.

Tl; DR
The upshot here is that after working to put the new class in place, it becomes clear that the complexity isn’t worth the savings. I thought it’d save half or 2/3 the code and 1/3 is closer to how it turns out. So, read knowing that, which is not what I knew. Also, I’m not going to include the old class, since we didn’t get the savings. So ignore the next sentence.

For comparison, I”ll put the current Knowledge class at the end of the article. It’s just over 100 lines long, and if I put it in here you’d just ignore it anyway. Out new prototype promises to be much simpler in use. Under test, it looks like this:

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}')

It will need additional methods as_dictionary, and update, if I recall, and there will need to be special handling for energy, which we want to store in the object but not in the dictionary part, which is shared with the server side as an information exchange format.

I think I’ll approach this directly. First, move NewKnowledge class to its own file, called ‘new_knowledge.py’ for now.

There are 27 usages of the Knowledge class, all but one of them in tests. We’ll wind up with all of them wired to this new class, if we keep it, because I plan to rename it to Knowledge after we get rid of the old one.

Install it in Bot and see what breaks.

We immediately see that the Knowledge creator accepts, nay demands, a location and direction. Fix:

class NewKnowledge:
    def __init__(self, location=None, direction=None):
        self._dict = dict()
        self.location = location
        self.direction = direction

28 tests break. Allow for location and direction in the list of allowed inputs:

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

Down to 24. See what the failures are saying. With any luck at all, they are all running into a disallowed Knowledge key.

The first one I look at is update, the one I least want to do. Let’s find some easy ones, if we can, and do them first. No, a raft of them are due to update, let’s bite the bullet. I decide to copy the existing update from Knowledge. It should work.

Note
When this gets too bad, I fully intend to roll back. But there’s a good chance that it won’t get bad at all. We do have issues, such as vision and energy but I expect them to be OK.
    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']

24 still fail but now, I hope, for simpler reasons. One is failing on receive. In Knowledge, that’s now just:

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

For some reason, we’re keeping that in a separate member. Let’s not.

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

Still 24. I think it likely that we’ll have to update our known items list pretty soon. Ah, yes in fact, of course, ow it wants held_entity in the list. Still 24, now it’s asking about _scent. We already have scent

I just noticed that we are setting to _scent in the update. Let’s use scent. Still 24. Now it wants vision. In Knowledge, vision is special: we wrap the return from world in a Vision object. We can do that in update:

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

We’re down to 17. I love it when a plan comes together.

We need a gain_energy method, and the other energy methods. We want energy to be in the class members, I think, not our dictionary.

Reflection
We could just leave everything in the dictionary and either cull out the ones we don’t want in the as_dictionary method, or possibly even leave them in and have a rule that the world will ignore keys it doesn’t care about. The fist solution would be better.

Let’s try putting everything in the dictionary, at least for now. I think it’ll be mostly harmless and get us to green sooner. We do still need the energy methods. Copying them over won’t quite do:

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

    def use_energy(self):
        self._energy = 0

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

We can call it _energy, and now that I think of it, that might be an easy way to distinguish things that shouldn’t go back to world in as_dictionary. But we cannot increment it quite like that. I do this:

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

    def use_energy(self):
        self._energy = 0

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

Still 17. We’re 40 minutes in but most of that is in writing not programming. Continue.

Ah, we need to init that value to zero, it seems.

class NewKnowledge:
    def __init__(self, location=None, direction=None):
        self._dict = dict()
        self.location = location
        self.direction = direction
        self.use_energy()

Still 17, this is getting tedious and slightly worrisome. I’d like to see some more progress here pretty soon.

We’re missing the energy constants that Knowledge includes. Copy them over:

class NewKnowledge:
    drop_threshold = 4
    take_threshold = 7
    energy_threshold = 5

    def __init__(self, location=None, direction=None):
        self._dict = dict()
        self.location = location
        self.direction = direction
        self.use_energy()

Only 13 tests failing now, how nice.

Ah, we still need the receive method, it is used from the outside. OK, no biggie. I add that, and remove, on the grounds that yeah we are gonna need it.

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

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

Still 13 though. We need has_block, not a surprise. It is a property.

    @property
    def has_block(self):
        return self.held_entity is not None

Down to 7(!). (Didn’t want you to think it was 7 factorial.)

We need has also.

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

Down to four! (Still not factorial.)

Just to show you what most of the errors look like:

    def __getattr__(self, item):
>       return self._dict[item]
E       KeyError: '_scent'

We’re using scent rather than _scent right now, as it was one that I added during testing. Let’s try changing it to _scent. Still 4. Why now?

One of the failures is looking for holding. This seems to be a convenience property covering held entity. I’ll add it but we’re going to want to simplify some of this later.

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

Down to just two failures. One of them is in the tests for the dictionary itself. We no longer accept scent, requiring +scent. Change the test. Down to one, and I do not like it:

    def test_bot_cant_take_diagonally(self):
        world = World(10, 10)
        bot = DirectConnection(world).add_bot(5, 5)
        world.add_block(4, 4)
        world.add_block(6, 4)
        world.add_block(4, 6)
        world.add_block(6, 6)
        world.take_forward(bot)
>       assert not bot.has_block()
E       assert not True
E        +  where True = has_block()
E        +    where has_block = <bot.Bot object at 0x106d6f260>.has_block

class Bot:
    def has_block(self):
        return self._knowledge.has_block
Added in Post
The concern about returning the method was mistaken. There is an issue, partly due to the way zero works.

It looks to me as if we returned the method, not the result? No. I introduced the is not None that we saw above, and if I change it to match the original, the test passes:

    @property
    def has_block(self):
        return self.held_entity

I don’t see why that made a difference. I think we’d best explore this a bit. I’ll add a print or two and assert False, to make that test break.

Curiously when left in the original form, I’m getting a zero back, and I get a True with the is not None.

I’m going to plug the old Knowledge back in for a moment and see what it used to do. It returned a zero. I think that’s not as correct as it might be. But wait, maybe that’s an id?

Ah, in WorldEntity.as_dictionary:

    def as_dictionary(self):
        held = 0 if not self.holding else self.holding.id
        return {'direction': self.direction,
                'held_entity': held,
                'eid': self.id,
                'location': self.location,
                'scent': self.scent,
                'vision': self.vision}

So the correct formulation is is not 0.

    @property
    def has_block(self):
        return self.held_entity is not 0

We are green. And fairly happy, I might add. New Knowledge is 60 lines long, as opposed to 106, and I think we will be able to make it smaller.

I do wonder about as_dictionary though. We didn’t implement it, but it is a method on Knowledge. Is it never used, never tested? Or what? It is not used on the Bot side of things, interesting. Oh, sure, we don’t send all our stuff back to world, we just send a few things. Let’s quickly review DirectConnection to reassure ourselves.

Right, we only pass commands and the bot.id, no other information goes back. We do not need as_dictionary.

Added in Post
The following turns out not to be true, we couldn’t quite get away with the name changing. I’m glad that I decided to take a half-way step, because it made it easier to realize that the savings I had expected just aren’t going to materialize. It would have been better, of course, to have made that assessment right here. I think we had most of the info we needed to decide the payoff wasn’t as high as I thought. But no harm done, what follows didn’t take long.

In principle, we could remove Knowledge class and rename NewKnowledge to Knowledge. That’s scary, isn’t it? Let’s take a smaller step, renaming Knowledge to OldKnowledge, in place, not with a refactoring, and then rename NewKnowledge with a refactoring. That will leave all the current references to Knowledge, all those tests, pointing to the new one.

Six tests fail on the first rename. Now the second. Bad things happen, lots of tests fail and are not started and whatnot. I think it’ll be an import issue.

Ah. I change out all the from knowledge import Knowledge lines but there are now some tests failing in the file test_knowledge.py, all of which are testing decisions that the bot makes, like can_take. We have no other tests for those. Move them over.

I am getting a strange error. It is as if Python does not recognize my can_take property.

    def test_knowledge_take_decision(self):
        location = Location(10, 10)
        direction = Direction.NORTH
        knowledge = Knowledge(location, direction)
        knowledge.vision = [('B', 10, 9)]
        knowledge._scent = Knowledge.take_threshold
>       assert knowledge.can_take

    def __getattr__(self, item):
>       return self._dict[item]
E       KeyError: 'can_take'

It looks as if the compiler has not called __getattribute__, which should have found can_take.

class Knowledge:
    @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
Added in Post
It’s not that weird, just a misreading on my part. But perhaps the nervous feeling caused me to be a bit more reluctant to live with the complexity of this tricky class, so it may have helped me make the decision that follows, i.e. to ditch it.

I can probably work through this, but it’s not quite on. I want to go back to just before I renamed Knowledge to OldKnowledge. PyCharm’s local history does that for me quite nicely. I am green, and the code is using the existing old Knowledge class for now. I can safely commit this, including my new class. Commit: great progress with NewKnowledge. Is it going to be good enough to use?

Let’s sum up. NewKnowledge class is up to 60 lines and will hit about 66 or more before we’re done. The old class is 106 lines, and does not include anything clever.

Since we can remove as_dictionary as unused, call it 96. We can probably improve it a bit.

Saving 30 lines with code that clever just isn’t that useful. Nice idea, fun four hour exercise. The class is in the repo if we ever want it.

Belay NewKnowledge and its test: not worth it. Remove them and commit. remove NewKnowledge and its tests, savings not worth the complexity.

Summary

Well, that was a bit disappointing. I had thought that the new class would save at least half the lines of the old one, and maybe more. And it certainly would have had the characteristic of checking all the members for correctness. But the facts did not bear out the expectation. Or, to put it another way, I was mistaken about how much savings I’d get for a somewhat extra clever class.

So be it. I have perhaps three hours invested in the thing, and I know a bit more about how to do that kind of thing, and have a lot more familiarity with how the testing goes … and honestly rather a lot fo confidence in it. We got to see a lot of failures and in fixing them, well, I came to trust them more because of all the different ways they wee catching me out. Mind you, I don’t totally believe them. I still like to run the game.

I’d be wise to work out why that is, and see whether I can devise some tests that will reduce my desire to run the game. Of course, it is fun to watch, but I do run it once in a while for testing purposes. If it ever breaks, I’ll make a point of adding tests for whatever fails.

Sometimes what seems like a really good idea doesn’t pan out. If we can determine in a few hours whether it’s good, it can be worth it, to improve our code. We could still use some improvement in Knowledge: it’s irritatingly long for what little it does.

Perhaps we’ll try another way. Perhaps we’ll do something else entirely. Stop by next time to find out.