Kondoizing?

The Repo on GitHub

Let’s take a look at the code today, looking for things that do ‘spark joy’ and things that don’t. Intuition. Pattern recognition. Joy.

I’m not sure that “Kondoizing” is actually a word, but we’re going to take a page from Marie Kondo’s book, well, not literally what kind of monster would tear a page out of a book but anyway what we’re going to do today is look at our code and see which things about it spark joy, and which do not.

I think that today we’ll focus on the actual code. It’s true that many of our tests don’t spark joy, in fact quite the opposite, but they do serve their purpose and the production code is more important.

Let’s start somewhere that I think will spark joy, the little state machine we use to make decisions. (It will not surprise me if, even there, we find things to improve. Such is the nature of things around here.)

We find three related classes, one for each state:

class Walking:
    def action(self, _knowledge):
        return []

    def update(self, knowledge):
        if knowledge.has_energy():
            if knowledge.has_block:
                return Laden()
            else:
                return Looking()
        return Walking()


class Looking:
    def action(self, knowledge):
        if knowledge.can_take:
            return ['take']
        return []

    def update(self, knowledge):
        if knowledge.has_block:
            knowledge.use_energy()
            return Walking()
        else:
            return Looking()


class Laden:
    def action(self, knowledge):
        if knowledge.has_energy():
            if knowledge.can_drop:
                return ['drop']
        return []

    def update(self, knowledge):
        if not knowledge.has_block:
            knowledge.use_energy()
            return Walking()
        else:
            return Laden()

Honestly, I do like these. The action and update methods read well and seem to me to tell the story pretty well. There is a bit of oddness that is built into the situation: when we take an action, we cannot immediately see the results, So there is a send-receive cycle in between action and update, but if we read the code in the order written, we get the right idea.

There is one thing that I think could use a bit of improvement:

class Laden:
    def update(self, knowledge):
        if not knowledge.has_block:
            knowledge.use_energy()
            return Walking()
        else:
            return Laden()

`` An if with not is harder to read than one that goes the other way. PyCharm will fix this for me, I think.

    def update(self, knowledge):
        if knowledge.has_block:
            return Laden()
        else:
            knowledge.use_energy()
            return Walking()

Commit: tidying.

There’s always room for improvement. One more thing:

class Walking:
    def update(self, knowledge):
        if knowledge.has_energy():
            if knowledge.has_block:
                return Laden()
            else:
                return Looking()
        return Walking()

There is an odd thing going on here. It is partly invisible and tricky anyway. We want the Bot to wander a bit after picking up a block or dropping one off, so that it tends not to put things down right where it finds them. So when we pick up or drop off, we use_energy and then go to the Walking state, and stay there until the bot has_energy. It’s not made explicit where it gains energy. I think that’s done in Bot, somewhere around do_something.

class Bot:
    def do_something(self, connection):
        actions = []
        actions += self.update_for_state_machine()  # <===
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        actions += ['step']
        self.perform_actions(actions, connection)

    def update_for_state_machine(self):
        self._knowledge.gain_energy()  # <===
        if self.location == self._old_location:
            return self.change_direction()
        else:
            return []

Right. I suggest that we only want the Bot to gain energy when just Walking, so we can and should move that code to the Walking state. Let’s try it.

class Walking:
    def update(self, knowledge):
        knowledge.gain_energy()  # <===
        if knowledge.has_energy():
            if knowledge.has_block:
                return Laden()
            else:
                return Looking()
        return Walking()

The game runs OK but I noticed that when I cut the method call from update_for_state_machine, no tests failed. Apparently we have no test for this energy feature.

This does not spark joy. I wasn’t going to look at tests, but this situation cannot endure.

We have a number of Walking tests, much like this one:

    def test_walking_tired_without_block_keeps_walking(self):
        state = Walking()
        knowledge = FakeKnowledge(energy=0, has_block=False)
        c = state.update(knowledge)
        assert isinstance(c, Walking)

But we always use our fake knowledge class, which is OK, but we never check to see whether the energy gets updated.

If I add one line to the test, it succeeds unless I comment out the gain_energy call:

    def test_walking_tired_without_block_keeps_walking(self):
        state = Walking()
        knowledge = FakeKnowledge(energy=0, has_block=False)
        c = state.update(knowledge)
        assert isinstance(c, Walking)
        assert knowledge._energy == 1

But let’s not do it that way, let’s do a dedicated test.

    def test_can_walk_off_loss_of_energy(self):
        state = Walking()
        knowledge = FakeKnowledge(energy=0, has_block=False)
        for _ in range(Knowledge.energy_threshold-1):
            new_state = state.update(knowledge)
            assert isinstance(new_state, Walking)
        new_state = state.update(knowledge)
        assert isinstance(new_state, Looking)

We walk and walk and finally we have enough energy to change to Looking state. Passes with the gain_energy in, fails with it out. Perfect. Commit: added test for walking off tiredness.

Reflection

Even in code that we like, we can often find things to improve, like inverting the if, or making it more explicit where the energy comes from. And we can discover missing tests: I just happened to notice that nothing failed when I removed that line in preparation for moving it.

The thoughts that drove these observations didn’t come from a checklist, nor even from “hard thinking” or “deep inspection” of the code. I “just noticed” that not, and while often I might ignore a not, since we were in the code improvement frame of mind, it kind of pinged me and I improved it.

The energy thing crept into my mind even more subtly. As I was reading the code, I was kind of telling the story of how it works in my head, not planning to write any more, but in an explanation frame of mine, and it pinged me that we couldn’t see the gain_energy. I knew where it was (roughly) but today, without really reasoning about it I just found myself realizing that we could do better.

I have seen organizations that were deeply into code inspection, and they had giant checklists of the things to consider, and, presumably, people were supposed to go through those checklists while reading the code. And we can read the demands of some advisors, telling us that our code needs to be, what is it, CRUNCHY? ROBUST? SOLID? I don’t know, probably SOLID.

I could not tell you what SOLID stands for. If you know, and think about those things, more power to you, but I think that what makes us potentially great is when we have built up our intuition, our reflexes, our pattern recognition, to the point where we “just know” that something isn’t quite right. We may then drop into an analytical mode to sort it out.

After we spot something, then we might even pull Liskov Substitution Principle out of some orifice and use it to explain what we’re doing. Heck, even I do like to refer to the “Law of Demeter”, primarily because one can express it in cute naughty ways, and, of course, because it is one well-known name for the rule that says an object should only talk to its immediate collaborators, and shouldn’t go digging into their pockets for things.

I’m not saying not to have checklists. I am saying that the real power comes when we come to the code with an open mind and well-trained intuition. How do we get that? Well, a lot of coding, a lot of reading, a lot of thinking, and, if then work for you, a lot of checklists.

At the end, if we can quickly look at the code, or feel how it feels to work with it and realize “something’s wrong here”, we’ll do well.

Let’s look for something else and then get out of here.

Knowledge

We were just looking at some code that uses the Knowledge class, so let’s look at it. It’s over 100 lines long, so look it over and let’s see what we see:

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

My initial take is that there are a lot of properties. Maybe this is OK, the object holds “all” of a Bot’s knowledge. A slightly deeper look shows two kinds of properties:

Getters and Setters

There are getters and setters for members. Again, this may be OK as the various facts are accessed from various places. But do we really need so many setters? Might be worth a look.

Detailed Properties

We’ve used properties for things like has_block, or can_take, which might be better expressed as methods can_take() and so on. I think that I am somewhat inconsistent about this, never sure which way I’d like to go.

Why might it matter? Well, because I can recall, more than once in this program, either leaving off parens from a method that needed them, or putting them on when they were not needed. Either mistake causes some fairly odd error messages and often they slow me down until I realize the mistake.

Intertwingled

I almost didn’t notice that there are properties and methods and properties and methods intertwingled. That’s not ideal. some reordering might be called for.

So, at some future time, we might want to look at the whole properties picture, here in particular, but we should probably make a decision and stick with it.

But wait, there’s more. Look at these three methods:

    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']

Clearly these three methods have a need to stay in sync. If we rename a member, the other two methods may (or may not) need changing. If we add a new kind of knowledge, which surely we shall, each of these three needs to be changed to include that new item.

I can think of at least two angles we should look at. One is the possibility, since we seem to be using a dictionary in and a dictionary out, maybe the internal data structure should be a dictionary. The other is that, whether it’s a dictionary or a big record as it is now, there should be some Universal Source of Truth kind of thing, listing all the names that we’re supposed to have, and everyone should work from that Source of Truth.

I honestly don’t know offhand how I might do that … but a quick glance at those three methods makes it very clear that if a line were missing from one of those, we might never notice it until Something Very Bad Happens.

We’ll leave that for another day. Today’s point isn’t particular changes, is it? It’s the power of the quick glance, the power of intuition.

Summary

I truly enjoy spotting things in the code and improving them. It sparks joy for me. It’s almost worth writing sloppy code. Fortunately I do not have to put in extra work to write code that can be improved. I think the fundamental point of programming is to write code that can be improved.

Rules, mantras, guidelines, mnemonics, litanies, and checklists notwithstanding, we will do well to pay attention to our intuition as we look at the code. We have a chance to train our intuition every time we reflect. We’ll do well to remain relaxed as we work, so that we can hear that little voice saying “that looks weird”, or “why is that there”, or “why is that one line different”.

We cannot think all the thoughts at the same time, not with conscious linear thought. But our brain is processing all kinds of patterns all the time, and it is sitting there whispering to us all the time. When the voices tell us to do evil things, we probably shouldn’t listen. But when they ask us what is up with our code, listening pays off.

Pattern recognition For The Win. Linear thinking is almost a sideline with the brain. The brain is fundamentally a pattern-recognizing machine. Let’s use it that way.