We’re going to do the marks yet again! Why do I do this? And: a digression on “AI”, and a very nice refactoring sequence. Also: Resist fascism, and take care of one another!

AI Digression

Nice Refactoring Sequence

To answer the question first: I do this for at least three reasons:

  1. It’s fun. I enjoy finding multiple ways to express the solution to the same problem;

  2. It increases my facility with the language, and “speaking” one’s programming language well makes everything go better;

  3. Good designs take big problems and reduce them to a set of smaller problems, so that practice in finding ways of solving small problems pays off over and over again.

In the case of our marks, specifically but not limited to the three mark boxes in the tenth frame, we have well-tested working code that doesn’t seem pleasing to me:

    def marks(self):
        # return a string containing all three marks (normal + tenth)
        return ''.join([self.first_mark(), self.second_mark(), self.third_mark()])

    def first_mark(self):
        if len(self._scores) < 1:
            return ' '
        score = self._scores[0]
        return self.possible_X(score)

    def second_mark(self):
        if len(self._scores) < 2:
            return ' '
        prior_roll = self._scores[0]
        current_roll = self._scores[1]
        return self.possible_slash_or_X(prior_roll, current_roll)

    def third_mark(self):
        if len(self._scores) < 3:
            return ' '
        prior_roll = self._scores[1]
        current_roll = self._scores[2]
        return self.possible_slash_or_X(prior_roll, current_roll)

    def possible_slash_or_X(self, prior_roll, current_roll):
        if prior_roll == 10:
            result = self.possible_X(current_roll)
        elif prior_roll + current_roll == 10:
            result = '/'
        else:
            result = self.possible_X(current_roll)
        return result

    def possible_X(self, score):
        if score == 10:
            return 'X'
        elif score == 0:
            return '-'
        else:
            return str(score)

What’s not to like? Well, first of all, it’s 40 lines of code to compute three characters. Second, there’s that repeated trope of skipping out with a blank, after checking the number of rolls one has available. Third, there’s something odd about there being so many calls to possible_X. Fourth, and perhaps most important: it’s not obviously correct.

So we’re going to do it again, differently. My plan this time is to store the marks as members of Box, and to calculate them during execution of the state machine. For example, if we roll a strike, we know right then that the first mark is an X, and if we do not, we know that it is a digit or dash (for zero), though we do not know yet what it is.

We’re on a green bar and a fresh commit. So I’m going to break the marks and then make them work again.

class Box:
    def __init__(self, previous_box = None):
        self.previous_box = previous_box if previous_box else ZeroBox()
        self._scores = []
        self._next_action = self._record_first_roll
        self._mark_1 = ' '
        self._mark_2 = ' '
        self._mark_3 = ' '
        self._complete = (self._strike_complete, self._spare_complete, self._open_frame_complete)

    def marks(self):
        # return a string containing all three marks (normal + tenth)
        return ''.join([self._mark_1, self._mark_2, self._mark_3])

I removed all the rest, even the helper methods, so that I won’t be tempted to use them if they’re just close to what I need. Now let’s see what we can figure out inside the state machine. It starts here:

    def _record_first_roll(self, pins):
        self._scores.append(pins)
        if pins == 10:
            self._next_action = self._record_strike_bonus_1
        else:
            self._next_action = self._record_second_roll
        return True

Well, how about that? If we have a ten, the first mark is an X. Otherwise it is a “digit” with the usual caveat that 0 becomes dash. Oh! I just had a neat idea. Wait until you see it, I hope you’ll like it.

    def _record_first_roll(self, pins):
        self._scores.append(pins)
        if pins == 10:
            self._mark_1 = 'X'
            self._next_action = self._record_strike_bonus_1
        else:
            self.mark_1 = '-123456789'[pins]
            self._next_action = self._record_second_roll
        return True

Isn’t that subscripting much nicer than the procedural code I’ve been using? Convert Procedure to Table Lookup pattern or something.

Let’s carry on down the strike path.

    def _record_strike_bonus_1(self, pins):
        self._scores.append(pins)
        self._next_action = self._record_strike_bonus_2
        return False

If we’re on a strike and the second roll is 10, we mark another X, otherwise a digit.

    def _record_strike_bonus_1(self, pins):
        self._mark_2 = 'X' if pins == 10 else '-123456789'[pins]
        self._scores.append(pins)
        self._next_action = self._record_strike_bonus_2
        return False

We’ve only got ten tests failing now, down from 14. Let’s see, what happens on the second bonus roll of a strike?

    def _record_strike_bonus_2(self, pins):
        self._scores.append(pins)
        self._next_action = self._strike_complete
        return False

This one is a bit more conditional. If mark_2 is x and pins is 10, we score X, otherwise digit …

    def _record_strike_bonus_2(self, pins):
        if self._mark_2 == 'X':
            self._mark_3 = 'X' if pins == 10 else '-123456789'[pins]
        self._scores.append(pins)
        self._next_action = self._strike_complete
        return False

And if we do not have an X in mark_2, since we’re on an initial strike, we could get a spare in mark_3 or digits, so let’s try this:

    def _record_strike_bonus_2(self, pins):
        if self._mark_2 == 'X':
            self._mark_3 = 'X' if pins == 10 else '-123456789'[pins]
        else:
            spare = pins + self._scores[-1]
            self._mark_3 = '/' if spare == 10 else '-123456789'[pins]
        self._scores.append(pins)
        self._next_action = self._strike_complete
        return False

Down to only five failures. PyCharm put an X where I have that / there … it is very good at predicting what I’m going to do but not perfect, and I didn’t notice the X. When looking at test failures, I was lucky to notice one that “should” have passed. We must digress:

Danger, Will Robinson!

I do not have the “AI” turned on in PyCharm, and other than perhaps experimenting with it so that I can speak with some knowledge, I do not intend to use it. The reason is that current AI systems are trained on copyrighted material (including mine), consume vast electric and water resources, and are already being used to the disadvantage of humans. Since I am nominally human, I side with the humans on this.

However, the whole point of an LLM-based “AI” system is to produce plausible text. I have seen LLMs produce perfectly reasonable looking code, except that it relies on library methods that simply do not exist. That’s going to be easy to find when you try to use it. But something as subtle as that ‘X’ where ‘/’ should be … that’s going to slip into the code, and it’s going to be hard to find.

I assert without proof but with lots of experience that finding bugs is much harder than preventing them. Finding them in someone else’s code? Not even fun.

It’s your choice whether and when to use an AI assistant. I’d suggest keeping in mind the economic cost, the human cost, and the cost of error.

But I digress. What tests are failing now?

None of them start with a strike, so let’s go down the spare path:

    def _record_first_roll(self, pins):
        self._scores.append(pins)
        if pins == 10:
            self._mark_1 = 'X'
            self._next_action = self._record_strike_bonus_1
        else:
            self.mark_1 = '-123456789'[pins]
            self._next_action = self._record_second_roll
        return True

    def _record_second_roll(self, pins):
        self._scores.append(pins)
        if sum(self._scores) == 10:
            self._next_action = self._record_spare_bonus
        else:
            self._next_action = self._open_frame_complete
        return True

Oh, wow! Did you see that self.mark_1 that should b self._mark_1? Neither did I, until just now. I noticed it because I was browsing the failing tests and noticed that I didn’t get a dash where I should have, at the beginning of one of the failing ones. OK, with that fixed, moving right along …

    def _record_second_roll(self, pins):
        self._scores.append(pins)
        if sum(self._scores) == 10:
            self._mark_2 = '/'
            self._next_action = self._record_spare_bonus
        else:
            self._mark_2 = '-123456789'[pins]
            self._next_action = self._open_frame_complete
        return True

Down to three failing now. What are they?

0 10 10 -> - / X
6 4 10 -> 6 / X
6 4 3 -> 6 / 3

I think we’ll find our next move in _record_spare_bonus. Sure enough:

    def _record_spare_bonus(self, pins):
        self._mark_3 = '0123456789X'[pins]
        self._scores.append(pins)
        self._next_action = self._spare_complete
        return False

It just occurred to me there that we can look up the X or / just as well as the digits.

All tests are green: Commit: computing marks during state machine execution.

But with this new insight into how indexing works, we can improve a bit:

    def _record_strike_bonus_1(self, pins):
        self._mark_2 = 'X' if pins == 10 else '-123456789'[pins]
        self._scores.append(pins)
        self._next_action = self._record_strike_bonus_2
        return False

That becomes:

    def _record_strike_bonus_1(self, pins):
        self._mark_2 = '-123456789X'[pins]
        self._scores.append(pins)
        self._next_action = self._record_strike_bonus_2
        return False

Similarly, we get:

    def _record_strike_bonus_2(self, pins):
        if self._mark_2 == 'X':
            self._mark_3 = '-123456789X'[pins]
        else:
            spare = pins + self._scores[-1]
            self._mark_3 = '/' if spare == 10 else '-123456789'[pins]
        self._scores.append(pins)
        self._next_action = self._strike_complete
        return False

We can’t do the second one, it’s checking the sum of two rolls to generate its slash.

Here, however:

    def _record_first_roll(self, pins):
        self._scores.append(pins)
        if pins == 10:
            self._mark_1 = 'X'
            self._next_action = self._record_strike_bonus_1
        else:
            self._mark_1 = '-123456789'[pins]
            self._next_action = self._record_second_roll
        return True

We can do this:

    def _record_first_roll(self, pins):
        self._scores.append(pins)
        self._mark_1 = '-123456789X'[pins]
        if pins == 10:
            self._next_action = self._record_strike_bonus_1
        else:
            self._next_action = self._record_second_roll
        return True

So that’s nice. I don’t see a good way to eliminate the if for the spare case. Life is tough, I guess.

We remain green. Commit: simplify cases that can just do string lookup.

Reflection

The Box class is reduced by 24 lines, from 113 to 89. The code for computing the marks is much less procedural, with two kinds of saving. First, moving the decisions into the various states eliminated some top-level conditional checks, and then the nice little table lookup, especially the one that includes the X in the input string, saves even more.

I would agree that you still have to think a bit to see if things are correct: the string lookup itself will take a moment to decode. But once you get it, you’ve got it everywhere it appears.

One interesting question has to do with the strings themselves ‘-123456789X’ and ‘-123456789’. They are used more than once and in any case they are weird constants. There’s a good argument that they be given meaningful names, and some would even argue for giving the lookup a named method. Let’s try that to see what we think:

    def _record_first_roll(self, pins):
        self._scores.append(pins)
        self._mark_1 = '-123456789X'[pins]
        if pins == 10:
            self._next_action = self._record_strike_bonus_1
        else:
            self._next_action = self._record_second_roll
        return True

If that '-123456789X'[pins] were to be a named method, what would the method be? Maybe just _mark? (It is surely private.)

    def _record_first_roll(self, pins):
        self._scores.append(pins)
        self._mark_1 = self._mark(pins)
        if pins == 10:
            self._next_action = self._record_strike_bonus_1
        else:
            self._next_action = self._record_second_roll
        return True

    @staticmethod
    def _mark(pins):
        return '-123456789X'[pins]

That can be used in several places, which PyCharm find. I had one occurrence where I had typed ‘0123…’ instead of ‘-123’, and PyCharm of course did not mention that. I found it by accident and fixed it. No test had failed. We “should” write one but I think we won’t. In any case, first there is this situation:

    def _record_second_roll(self, pins):
        self._scores.append(pins)
        if sum(self._scores) == 10:
            self._mark_2 = '/'
            self._next_action = self._record_spare_bonus
        else:
            self._mark_2 = '-123456789'[pins]
            self._next_action = self._open_frame_complete
        return True

    def _record_strike_bonus_2(self, pins):
        if self._mark_2 == 'X':
            self._mark_3 = self._mark(pins)
        else:
            spare = pins + self._scores[-1]
            self._mark_3 = '/' if spare == 10 else '-123456789'[pins]
        self._scores.append(pins)
        self._next_action = self._strike_complete
        return False

Refactoring Sequence

A rather nice refactoring sequence starts here. We’re going to make these two methods more and more alike until finally we collapse out the duplication. Maybe try it yourself: it’s fun!

We have some rather subtle duplication going on here, the handling of the spare case. Both methods are emitting / if the sum of some scores is 10 and otherwise looking up the digit.

There’s another thing that I don’t like here. The appending of the pins to _scores happens early on in the methods in some places and later in others. That can only lead to trouble. Let’s do it first, always.

With that in place and a bit of making things look more similar, we have this:

    def _record_second_roll(self, pins):
        self._scores.append(pins)
        if sum(self._scores[-2:]) == 10:
            self._mark_2 = '/'
            self._next_action = self._record_spare_bonus
        else:
            self._mark_2 = '-123456789'[pins]
            self._next_action = self._open_frame_complete
        return True

    def _record_strike_bonus_2(self, pins):
        self._scores.append(pins)
        if self._mark_2 == 'X':
            self._mark_3 = self._mark(pins)
        else:
            spare = sum(self._scores[-2:])
            self._mark_3 = '/' if spare == 10 else '-123456789'[pins]
        self._next_action = self._strike_complete
        return False

Let’s duplicate the if block in _record_second_roll, making one block for the mark and one for the state:

    def _record_second_roll(self, pins):
        self._scores.append(pins)
        is_spare = sum(self._scores[-2:]) == 10
        if is_spare:
            self._mark_2 = '/'
        else:
            self._mark_2 = '-123456789'[pins]
        if is_spare:
            self._next_action = self._record_spare_bonus
        else:
            self._next_action = self._open_frame_complete
        return True

Green, by the way. I should be committing this. Commit: making duplication.

Now compare the method above to this one:

    def _record_strike_bonus_2(self, pins):
        self._scores.append(pins)
        if self._mark_2 == 'X':
            self._mark_3 = self._mark(pins)
        else:
            spare = sum(self._scores[-2:])
            self._mark_3 = '/' if spare == 10 else '-123456789'[pins]
        self._next_action = self._strike_complete
        return False

Let’s ternary the top one:

    def _record_second_roll(self, pins):
        self._scores.append(pins)
        is_spare = sum(self._scores[-2:]) == 10
        self._mark_2 = '/' if is_spare else '-123456789'[pins]
        if is_spare:
            self._next_action = self._record_spare_bonus
        else:
            self._next_action = self._open_frame_complete
        return True

Now two step, starting here:

    def _record_strike_bonus_2(self, pins):
        self._scores.append(pins)
        if self._mark_2 == 'X':
            self._mark_3 = self._mark(pins)
        else:
            spare = sum(self._scores[-2:])
            self._mark_3 = '/' if spare == 10 else '-123456789'[pins]
        self._next_action = self._strike_complete
        return False

Inline spare to here:

    def _record_strike_bonus_2(self, pins):
        self._scores.append(pins)
        if self._mark_2 == 'X':
            self._mark_3 = self._mark(pins)
        else:
            self._mark_3 = '/' if sum(self._scores[-2:]) == 10 else '-123456789'[pins]
        self._next_action = self._strike_complete
        return False

Extract variable including the == 10, to here:

    def _record_strike_bonus_2(self, pins):
        self._scores.append(pins)
        if self._mark_2 == 'X':
            self._mark_3 = self._mark(pins)
        else:
            is_spare = sum(self._scores[-2:]) == 10
            self._mark_3 = '/' if is_spare else '-123456789'[pins]
        self._next_action = self._strike_complete
        return False

I could and should commit on each of those steps. Now the code above looks suspiciously similar to this:

    def _record_second_roll(self, pins):
        self._scores.append(pins)
        is_spare = sum(self._scores[-2:]) == 10
        self._mark_2 = '/' if is_spare else '-123456789'[pins]
        if is_spare:
            self._next_action = self._record_spare_bonus
        else:
            self._next_action = self._open_frame_complete
        return True

Extract a method on just the ternary line:

    def _record_second_roll(self, pins):
        self._scores.append(pins)
        is_spare = sum(self._scores[-2:]) == 10
        self._mark_2 = self._mark_possible_spare(is_spare, pins)
        if is_spare:
            self._next_action = self._record_spare_bonus
        else:
            self._next_action = self._open_frame_complete
        return True

    @staticmethod
    def _mark_possible_spare(is_spare, pins):
        return '/' if is_spare else '-123456789'[pins]

PyCharm finds and changes the other with my permission:

    def _record_strike_bonus_2(self, pins):
        self._scores.append(pins)
        if self._mark_2 == 'X':
            self._mark_3 = self._mark(pins)
        else:
            is_spare = sum(self._scores[-2:]) == 10
            self._mark_3 = self._mark_possible_spare(is_spare, pins)
        self._next_action = self._strike_complete
        return False

Commit: extract _mark_possible_spare.

I think that’s as good as it gets. I don’t feel right about using _mark in _mark_possible_spare, because it looks as if it could return an X but it really can’t. This code makes it more clear, I think.

Would you inline the is_spare just above? No, I didn’t think so.

That refactoring adds nine lines to the file, but I think the two mark methods make things more clear:

    @staticmethod
    def _mark(pins):
        return '-123456789X'[pins]

    @staticmethod
    def _mark_possible_spare(is_spare, pins):
        return '/' if is_spare else '-123456789'[pins]

Summary

In favor of this idea, by my lights:

  1. The code is smaller, but not made more obscure thereby;
  2. The code takes good advantage of the state machine’s states, reducing conditionals;
  3. The marks are computed at the same time, using the same logic, as the score.
  4. For that reason, the assignments made to the marks always make sense in the context.
  5. The string lookup is nice (though we could have done that before).

Against:

  1. Dispersing the mark logic into the state machine might make it harder to track just how the marks are computed,
  2. _mark_2 can be set in one of two places, and _mark_3 in one of three. Only one such setting can happen in any given frame, but you still have to look in multiple places.

I think I prefer this scheme by a small to medium margin. I value the little lookup trick. I like that when you see where a mark is set, it makes sense. And I would argue that doing the marks as part of the state machine is the “right” thing to do, compared to computing them after the fact.

In all, a pleasant interlude with a pleasant outcome. I wish everything were like that.

Resist fascism, take care of one another, write to your congress people, and come see me next time!