I do not like the code that fills the small boxes. I have the time, and I’m going to keep trying. We get some good results. Not done yet. Also: Resist fascism, and take care of one another.

Unfortunately, for many developers, once something seems to work, they feel required to move on to the next thing. I don’t think that’s the best habit to have, but my approach to things led to me having a KSR-33 teletype in my marital bedroom, which might not be ideal either. Be that as it may, I do have the time and the inclination to find a better way to fill those little boxes.

We do have some tests for them, including this new test for the tenth frame, including two supporting functions that made it easier to write the test:

    def get_tenth_frame(self, r1, r2, r3):
        s = Scorekeeper()
        for _frame in range(1,10):
            s.roll(0)
            s.roll(0)
        s.roll(r1)
        s.roll(r2)
        s.roll(r3)
        return s.frame(9)

    def get_boxes(self, f):
        return ''.join([
            f.tenth_frame(0),
            f.tenth_frame(1),
            f.tenth_frame(2)
        ])

    def test_tenth(self):
        f = self.get_tenth_frame(10, 6, 4)
        boxes = self.get_boxes(f)
        assert boxes == 'X6/'
        f = self.get_tenth_frame(10, 0, 10)
        assert self.get_boxes(f) == 'X-/'
        f = self.get_tenth_frame(0, 10, 10)
        assert self.get_boxes(f) == '-/X'
        f = self.get_tenth_frame(10, 10, 10)
        assert self.get_boxes(f) == 'XXX'

This morning, I think I see how to make the testing even easier, by combining those two functions into one that provides the rolls and the values. We’ll see about that. Let’s look at the code, starting with the normal frames, from the drawing on down:

class NormalFrameView:
    def draw_1_thru_9(self, game):
        self.draw_common_elements(game)
        self.draw_first_roll()
        self.draw_mark()

    def draw_first_roll(self):
        mark = self.frame.first_roll()
        if mark == 'X':
            return
        multiplier = 3
        self.vh.draw_in_box(self.f, mark, multiplier)

    def draw_mark(self):
        mark = self.frame.mark()
        multiplier = 5
        self.vh.draw_in_box(self.f, mark, multiplier)

class Frame:
    def first_roll(self):
        try:
            return self._first_roll()
        except Exception:
            return ''

    def _first_roll(self):
        s = self.start
        if self.strike((r := self.rolls[s])):
            return 'X'
        else:
            return self.dash_for_zero(r)

    def mark(self):
        try:
            return self._mark()
        except Exception:
            return ''

    def _mark(self):
        s = self.start
        rolls = self.rolls
        if self.strike(rolls[s]):
            return 'X'
        elif rolls[s] + (r2 := rolls[s + 1]) == 10:
            return '/'
        else:
            return self.dash_for_zero(r2)

That’s just a lot and there is that redundancy between the two methods, checking for strike. In addition, as I forgot to deal with yesterday, a strike mark goes in the left-hand (center) small box, not the right hand one.

Reading that code, I see that the view hacks the result from the frame, not drawing the X in the first box. We can fix the bug. Let’s do that first, so that if today’s attempt to do it right fails, we’ll have the bug fixed.

    def draw_first_roll(self):
        mark = self.frame.first_roll()
        multiplier = 3
        self.vh.draw_in_box(self.f, mark, multiplier)

That much should put an X in both boxes, and it does. We make a corresponding change here:

    def _mark(self):
        s = self.start
        rolls = self.rolls
        if self.strike(rolls[s]):
            return ''
        elif rolls[s] + (r2 := rolls[s + 1]) == 10:
            return '/'
        else:
            return self.dash_for_zero(r2)

A test breaks. That’s good news, it tells me that I have a test. It says:

    def test_mark(self):
        s = Scorekeeper()
        s.roll(10)
        s.roll(6)
        s.roll(4)
        s.roll(3)
        s.roll(2)
        assert s.frame(0).mark() == 'X'
        assert s.frame(1).mark() == '/'
        assert s.frame(2).mark() == '2'
        assert s.frame(3).mark() == ''

Change the X line:

        assert s.frame(0).mark() == ''

Passes. If I returned a Y instead of an X on the first box, would a test find the bug? I am afraid not, let’s find out. We have a test!

    def test_first_roll(self):
        s = Scorekeeper()
        s.roll(10)
        s.roll(6)
        assert s.frame(0).first_roll() == 'X'
        assert s.frame(1).first_roll() == '6'

OK, put the X back and commit: fix defect that put strike X in wrong box.

OK, let’s get to finding better code. I have these ideas which I think will combine to give us a good result:

  1. Return both strings for normal frames and all three for tenth frame, using one function (for each frame type) rather than the current two and three functions. This should reduce duplication.
  2. Have the dash_for_zero function convert 0 to dash and 10 to X, and other numbers to their string value. This may allow us to avoid checking some cases.
  3. Try using Python’s match-case statements. This may make the code more compact and readable, yet give greater confidence that all cases are handled.
  4. Provide safe functions that return the first, second, and third rolls, or None, for convenient use. This may reduce or even remove the need for the exception handling. I liked the exceptions when I though I just had to do it once, but there are now four try-except uses in Frame class.
  5. Consider making a wrapper object for Frame to do this work, allowing Frame to deal with the lower level issues including scoring, but not the more view-like notion of the strings that go into the view. I think I would NOT put those functions in the view. I think we’ll hold off on this idea until we’re done, then decide.

Let’s write some normal frame tests, positing a new method in Frame. Here’s my first cut:

    def test_normal_strings(self):
        def create_frame(*args):
            s = Scorekeeper()
            for r in args:
                s.roll(r)
            return s.frame(0)

        f = create_frame(10)
        center, right = f.get_normal_boxes(self)
        assert center == 'X'
        assert right == ''

This is enough to fail. It wants that get_normal_boxes function. We’ll try to oblige, using a bit of fakery to get going.

class Frame:
    def get_normal_boxes(self, frame):
        return 'X', ''

Perfect! Ship it! OK, maybe not yet. I’m going to hold off on committing until I’m fairly sure that I like this new scheme. History suggests that I might not.

Now I want those safe rolls. I propose to use properties.

    def test_properties(self):
        f = self.create_frame()
        assert f.first is None
        assert f.second is None
        assert f.third is None
        f = self.create_frame(1, 2, 3)
        assert f.first == 1
        assert f.second == 2
        assert f.third == 3

class Frame:
    @property
    def first(self):
        return self._roll(0)

    @property
    def second(self):
        return self._roll(1)

    @property
    def third(self):
        return self._roll(2)

    def _roll(self, number):
        if number < len(self.rolls):
            return self.rolls[number]
        else:
            return None

I think we’ll find that we can use those to some benefit elsewhere. Now make the box test harder:

    def test_normal_strings(self):
        f = self.create_frame(10)
        center, right = f.get_normal_boxes()
        assert center == 'X'
        assert right == ''
        f = self.create_frame(6, 4)
        center, right = f.get_normal_boxes()
        assert center == '6'
        assert right == '/'
        f = self.create_frame(6, 3)
        center, right = f.get_normal_boxes()
        assert center == '6'
        assert right == '3'

That’s failing, of course. Thinks everyone is always rolling strikes. We can do better:

class Frame:
    def get_normal_boxes(self):
        first, second = self.first, self.second
        match first, second:
            case 10, _:
                return 'X', ''
            case a, b if a + b == 10:
                return self.dash_for_zero(a), '/'
            case a, b:
                return self.dash_for_zero(a), self.dash_for_zero(b)

And the test is green. I feel good about this so far. Good enough to use it and commit it? I think yes.

The view currently calls first_roll and mark on Frame to get the strings. Let’s implement those to use this new scheme:

Ah, my properties are wrong! Weak test.

Arrgh. I’m in a bit of trouble. Something isn’t working as I think it should. When I make this change:

class Frame:
    def first_roll(self):
        b1, b2 = self.get_normal_boxes()
        return b1

These tests fail:

    def test_properties(self):
        f = self.create_frame()
        assert f.first is None
        assert f.second is None
        assert f.third is None
        f = self.create_frame(1, 2, 3)
        assert f.first == 1
        assert f.second == 2
        assert f.third == 3

    def test_first_roll(self):
        s = Scorekeeper()
        s.roll(10)
        s.roll(6)
        assert s.frame(0).first_roll() == 'X'
        assert s.frame(1).first_roll() == '6'

Undo that change.

The properties are not correct because I didn’t consider self.start. But they will not work as is if self.start is still None.

Fix that.

    def _roll(self, number):
        if self.start is None:
            return None
        elif number < len(self.rolls):
            return self.rolls[self.start + number]
        else:
            return None

That can be better. How about this:

    def _roll(self, roll_number):
        if self.start is None or roll_number >= len(self.rolls):
            return None
        else:
            return self.rolls[self.start + roll_number]

We’re green. Proceed to use the new match stuff in the View.

class Frame:
    def first_roll(self):
        b1, b2 = self.get_normal_boxes()
        return b1

A test fails!

    def test_first_roll(self):
        s = Scorekeeper()
        s.roll(10)
        s.roll(6)
        assert s.frame(0).first_roll() == 'X'
        assert s.frame(1).first_roll() == '6'

It’s raising a TypeError, and I see why: we have no case for the first roll present and second missing. Fix, adding the a, None case:

    def get_normal_boxes(self):
        first, second = self.first, self.second
        match first, second:
            case 10, _:
                return 'X', ''
            case a, None:
                return self.dash_for_zero(a), ''
            case a, b if a + b == 10:
                return self.dash_for_zero(a), '/'
            case a, b:
                return self.dash_for_zero(a), self.dash_for_zero(b)

This passes tests. Forgive me for looking at the GUI now.

Glad I did, it’s displaying None right across the board.

display full of "None"

Need more cases. I add one to my test:

        f = self.create_frame()
        center, right = f.get_normal_boxes()
        assert center == ''
        assert right == ''

That fails getting ‘None’, so. We have this:

    def get_normal_boxes(self):
        first, second = self.first, self.second
        match first, second:
            case 10, _:
                return 'X', ''
            case a, None:
                return self.dash_for_zero(a), ''
            case a, b if a + b == 10:
                return self.dash_for_zero(a), '/'
            case a, b:
                return self.dash_for_zero(a), self.dash_for_zero(b)

I think we could fix that with a check for two Nones (two Nones go into a bar …) but let’s do this instead:

    def box_display(self, roll):
        match roll:
            case None:
                return ''
            case 0:
                return '-'
            case 10:
                return 'X'
            case _:
                return str(roll)

    def get_normal_boxes(self):
        first, second = self.first, self.second
        match first, second:
            case 10, _:
                return 'X', ''
            case a, None:
                return self.box_display(a), ''
            case a, b if a + b == 10:
                return self.box_display(a), '/'
            case a, b:
                return self.box_display(a), self.box_display(b)

I renamed the dash_whatnot method to box_display and extended it, as suggested early on, to handle the case of 10, and also None.

The tests pass and the game displays correctly. I think the tests cover all the normal frame cases now. I’ll remove the dead code, leaving just this:

    def first_roll(self):
        return self.get_normal_boxes()[0]

Commit: first_roll method now uses get_normal_boxes.

We should be able to make the same change to mark. Tests are green. I check the GUI anyway. It’s good too.

Commit: mark uses get_normal_boxes.

Reflection

I’m nearly two hours in. Taking longer than I thought, but I’ve written a lot of words here. And I’ve made a lot of little errors that I spared you (or spared myself the embarrassment). And, one key thing, I went back to the more pythonic form of this method, using exceptions:

    def _roll(self, roll_number):
        try:
            return self.rolls[self.start + roll_number]
        except (IndexError, TypeError):
            return None

We get a type error if self.start is None, which it can be before there are any rolls, and an index error if we ask for a roll beyond the number available. A case could be made for initializing start to zero rather than None, but None seemed better at the time.

Before we start on the tenth frame boxes, let’s look around to see if there are places that can use our new properties first, second, and third. (I freely grant that third is speculative and not currently of much value. I make decisions and live with them. I’m OK with writing code that I might not need, though I do try to do very little of it.)

It seems to me that we could use the properties here:

    def frame_score(self):
        try:
            return self._frame_score()
        except Exception:
            return None

    def _frame_score(self):
        s = self.start
        rolls = self.rolls
        first_roll = rolls[s]
        if self.strike(first_roll):
            return 10 + rolls[s+1] + rolls[s+2]
        second_roll = rolls[s+1]
        if (two := first_roll + second_roll) != 10:
            return two
        return two + rolls[s+2]

First, I’ll use them in the _ method and then combine upward.

    def frame_score(self):
        try:
            return self._frame_score()
        except Exception:
            return None

    def _frame_score(self):
        if self.strike(self.first):
            return 10 + self.second + self.third
        if (two := self.first + self.second) != 10:
            return two
        return two + self.third

I think this can still raise an exception, so we’re stuck with this much. I check that, and yes we can get an exception trying to add None to a number, Still, better than it was. Commit: applying first, second, third.

Summary

Let’s pause here, as this article is too long to read already. We’re in a better place, with a simpler approach to the small boxes in the normal frame, and some simplified code in scoring appearing as a side benefit. I’ll take a little break, or perhaps a big one and then return to work on the tenth frame next time.

Oh but let’s rename the NormalFrameView to FrameView, since there is just the one. That takes about nine keystrokes to do and commit. Well worth it.

We can probably improve that class by fetching both the boxes at the same time. We’ll look at that in the future. I’m ready for chai now.

See you next time. Resist fascism, take care of one other, and write to your representatives.