I have a few moments at the code face, and see a couple of possible improvements. Also: Resist fascism, and take care of one another.

I started by observing that the two View classes are getting quite similar now in their draw methods:

class NormalFrameView:
    def __init__(self, game, frame_number, frame: Frame):
        self.game = game
        self.f = frame_number
        self.frame = frame

    def draw(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        vh = ViewHelper(game)
        vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        vh.small_box(x_right, mark_line, 1)
        vh.draw_frame_total(self.f, self.frame)
        self.draw_first_roll()
        self.draw_mark()

class TenthFrameView:
    def __init__(self, game, frame: Frame):
        self.game = game
        self.frame = frame
        self.f = 9

    def draw(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        vh = ViewHelper(game)
        vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        vh.small_box(x_right, mark_line, 1)
        vh.small_box(x_right, mark_line, 2)
        vh.small_box(x_right, mark_line, 3)
        vh.draw_frame_total(self.f, self.frame)
        self.draw_tenth_frame(0)
        self.draw_tenth_frame(1)
        self.draw_tenth_frame(2)

I’m thinking that with a few not too terrible conditional statements, we could combine those two classes into one.

And that caused me to look at the ViewHander, with actually a couple of notions coming to mind. First, why not just create a single instance in the View, and reuse it throughout, and, second, why not give it the frame number, so that it can do its own subscript calculations.

Let’s do that first, in both classes. First, build it as is and simply use it:

class NormalFrameView:
    def __init__(self, game, frame_number, frame: Frame):
        self.game = game
        self.f = frame_number
        self.frame = frame
        self.vh = ViewHelper(self.game)

I’m wondering, as I look at that, why we wouldn’t pass it the View rather than the game, since the view knows the game. Anyway, one thing at a time.

I quickly change NormalFrameView to use only self.vh throughout. Commit: create and use member for ViewHelper.

It seems a bit wasteful, given that I plan to get rid of the TenthFrameView entirely, but I think it’s better to install the member ViewHandler first. It should make moving the code into NormalFrameView easier. So we’ll do that.

Works fine, commit, same message.

A Question

What is a good, simple, small-step, safe way to move all the tenth frame’s function into ninth?

Oh, here’s an interesting notion: extract a method from each one’s draw, like this:

class NormalFrameView:
    def draw(self, game):
        self.draw_1_thru_9(game)

    def draw_1_thru_9(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.vh.small_box(x_right, mark_line, 1)
        self.vh.draw_frame_total(self.f, self.frame)
        self.draw_first_roll()
        self.draw_mark()

class TenthFrameView:
    def draw(self, game):
        self.draw_10(game)

    def draw_10(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.vh.small_box(x_right, mark_line, 1)
        self.vh.small_box(x_right, mark_line, 2)
        self.vh.small_box(x_right, mark_line, 3)
        self.vh.draw_frame_total(self.f, self.frame)
        self.draw_tenth_frame(0)
        self.draw_tenth_frame(1)
        self.draw_tenth_frame(2)

Now I can merge the two classes easily:

class NormalFrameView:
    def draw(self, game):
        if self.f < 9:
            self.draw_1_thru_9(game)
        else:
            self.draw_10(game)

class Game:
    def __init__(self, scorekeeper: Scorekeeper):
        self.scorekeeper = scorekeeper
        pygame.init()
        self.running = False
        self.screen = pygame.display.set_mode(self.WINDOW_SIZE)
        pygame.display.set_caption('Bowling')
        self.frames: List[Union[NormalFrameView, TenthFrameView]]  = list()
        for i, frame in enumerate(self.scorekeeper.frames):
            view = NormalFrameView(self, i, frame)
            self.frames.append(view)

The latter bit used to create nine Normal and one Tenth. I just moved the two methods over from Tenth and we’re good. Remove the class entirely, change the list definition and remove the import:

class Game:
    def __init__(self, scorekeeper: Scorekeeper):
        self.scorekeeper = scorekeeper
        pygame.init()
        self.running = False
        self.screen = pygame.display.set_mode(self.WINDOW_SIZE)
        pygame.display.set_caption('Bowling')
        self.frames: List[NormalFrameView]  = list()
        for i, frame in enumerate(self.scorekeeper.frames):
            view = NormalFrameView(self, i, frame)
            self.frames.append(view)

Everything works. Commit: remove TenthFrameView, Normal handles all cases.

Now we have everything in one class with two large draw methods that contain a lot of similarity:

class NormalFrameView:
    def draw(self, game):
        if self.f < 9:
            self.draw_1_thru_9(game)
        else:
            self.draw_10(game)

    def draw_1_thru_9(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.vh.small_box(x_right, mark_line, 1)
        self.vh.draw_frame_total(self.f, self.frame)
        self.draw_first_roll()
        self.draw_mark()

    def draw_10(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.vh.small_box(x_right, mark_line, 1)
        self.vh.small_box(x_right, mark_line, 2)
        self.vh.small_box(x_right, mark_line, 3)
        self.vh.draw_frame_total(self.f, self.frame)
        self.draw_tenth_frame(0)
        self.draw_tenth_frame(1)
        self.draw_tenth_frame(2)

PyCharm notices six lines of duplication so we extract that. Extracting method does an interesting thing, returning the two values needed:

    def draw_1_thru_9(self, game):
        mark_line, x_right = self.draw_main_box(game)
        self.vh.draw_frame_total(self.f, self.frame)
        self.draw_first_roll()
        self.draw_mark()

    def draw_10(self, game):
        mark_line, x_right = self.draw_main_box(game)
        self.vh.small_box(x_right, mark_line, 2)
        self.vh.small_box(x_right, mark_line, 3)
        self.vh.draw_frame_total(self.f, self.frame)
        self.draw_tenth_frame(0)
        self.draw_tenth_frame(1)
        self.draw_tenth_frame(2)

    def draw_main_box(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.vh.small_box(x_right, mark_line, 1)
        return mark_line, x_right

We’ll undo, move it up to increase the duplication, then redo the extract. After the move:

    def draw_1_thru_9(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.vh.draw_frame_total(self.f, self.frame)
        self.vh.small_box(x_right, mark_line, 1)
        self.draw_first_roll()
        self.draw_mark()

    def draw_10(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.vh.draw_frame_total(self.f, self.frame)
        self.vh.small_box(x_right, mark_line, 1)
        self.vh.small_box(x_right, mark_line, 2)
        self.vh.small_box(x_right, mark_line, 3)
        self.draw_tenth_frame(0)
        self.draw_tenth_frame(1)
        self.draw_tenth_frame(2)

Before we extract, how do draw_first_roll and draw_mark differ from draw_tenth_frame?

    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)

    def draw_tenth_frame(self, n):
        mark = self.frame.tenth_frame(n)
        step = 2*n + 1
        self.vh.draw_in_box(self.f, mark, step)

Well, I think there is meat there to be found but we’ll put that off. Extract the seven lines that totally match:

    def draw_1_thru_9(self, game):
        self.daw_common_elements(game)
        self.draw_first_roll()
        self.draw_mark()

    def draw_10(self, game):
        mark_line, x_right = self.daw_common_elements(game)
        self.vh.small_box(x_right, mark_line, 2)
        self.vh.small_box(x_right, mark_line, 3)
        self.draw_tenth_frame(0)
        self.draw_tenth_frame(1)
        self.draw_tenth_frame(2)

    def draw_common_elements(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.vh.draw_frame_total(self.f, self.frame)
        self.vh.small_box(x_right, mark_line, 1)
        return mark_line, x_right

That’ll do. Commit: draw common elements with extracted method.

You know what? Let’s move the other two small boxes down into the common elements and skip them except on frame 10.

    def draw(self, game):
        if self.f < 9:
            self.draw_1_thru_9(game)
        else:
            self.draw_10(game)

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

    def draw_10(self, game):
        self.draw_common_elements(game)
        self.draw_tenth_frame(0)
        self.draw_tenth_frame(1)
        self.draw_tenth_frame(2)

    def draw_common_elements(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.vh.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.vh.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.vh.draw_frame_total(self.f, self.frame)
        self.vh.small_box(x_right, mark_line, 1)
        if self.f == 9:
            self.vh.small_box(x_right, mark_line, 2)
            self.vh.small_box(x_right, mark_line, 3)

I like that better: the two top methods make more sense to me. The “common_elements” name isn’t quite accurate, but we’ll let it live for now. Commit: move small boxes all down to common drawing.

Summary

With a few simple and relatively safe moves, we have reduced two classes to one, and two very large methods to four methods not much larger than one of the preceding two. A noticeable improvement.

Again, we see that small steps can improve our code. Fascinating.

See you next time, and remember to resist fascism, and take care of one another.