FGNO Observations

Last night was FGNO. We spoke at length about this simple exercise. Herewith, some observations. Chet’s blackmail plan thwarted. Also: Resist fascism, and take care of one another.

FGNO, Friday Geeks Night Out, occurs every Tuesday evening, after we quickly discovered that Friday night doesn’t work well for an evening meeting, between date night, drinking night, and letdown after a weeks’ work, for the [un]fortunates who work for a living.

Last night, of the six attendees, four had worked on Hill’s proposed variation of the famous bowling game exercise, namely making the visible scoring interface a critical aspect of the exercise. The usual exercise is simply an exercise of building and refactoring the model side of the code, because it’s about the right size for an early learning exercise or demonstration. Adding in the GUI stresses the process a lot:

Customer Interaction

Working with the GUI focused “customer” results in early identification of requirements that can guide the implementation and that might be troublesome if discovered later on in the process.
We all recognized that having even an imaginary customer with us enabled fast iteration on the look and feel, and observed that it felt time-consuming just because the GUI tool sets are what they are.
GeePaw observed that frequent demonstrations are a “sales” technique, in that they keep the customer feeling satisfied that progress is being made. This wasn’t new: we know that’s part of why we do incremental development. But couching it in terms of “sales” rather than just “feedback” puts focus on why we do it, and, I think, on why the demo needs to be no more compelling than the whole product supports. We don’t want the demo to lie about the project status.
Chet suggested, and we all agreed, that there is great value to having the customer present essentially all the time instead of just for a few meetings. More XP, less Scrum.

Project Impact

Most of the testing one can do on the GUI takes the form of looking at it, not liking it, and tweaking whatever cryptic methods make up your GUI framework until it looks better;
Connecting the GUI to the simple bowling game we all know how to write turns out to be tricky: there are things we want to know about the game state that we have never been concerned about before.
Those of us with fresh code to review had all taken shortcuts, or left out good ideas, and left the program with places that could use a little improvement. (This isn’t necessarily bad: at any point in a program’s evolution, we are likely to see things that could be improved.)
The GUI can look good while the actual functionality is absent, weak, or messy. The demo can mislead the customer into feeling better about the project than they should.
I reported that I felt rushed, and knew that I was rushing, and knew that I was leaving the code in more of a mess than I a comfortable with … and that I went ahead and rushed anyway, because of the Tuesday night deadline.

It was a great session and we wished we had recorded it. We’re going to start recording all of them, until we get used to the fact that it’s happening and settle back into our real personalities instead of our “on camera” personalities. When, as sometimes happens, the session is worth publishing, we’ll edit and publish. Otherwise, I suppose, Chet would try to use them as blackmail material. Good luck with that, none of us has any money and most of us have no remaining reputation to tarnish.

Review

Let’s do a bit of a review on my code for this exercise. I’ve made a few changes and we’ll probably find a few more things that are worth mentioning.

I grabbed a little extra time somewhere and removed some duplication with a ViewHelper object:

class ViewHelper:
    def draw_frame_total(self, game: Game, frame_number, frame: Frame):
        total = frame.total_score()
        display_total = str(total) if total is not None else ""
        font = pygame.font.Font(game.BASE_FONT, 30)
        font.set_bold(True)
        text = font.render(display_total, True, game.FOREGROUND, game.BACKGROUND)
        text_rect = text.get_rect()
        text_rect.centerx = frame_number * game.FRAME_SIZE + game.FRAME_SIZE / 2
        text_rect.centery = game.GAME_Y - 20
        game.screen.blit(text, text_rect)

That’s used twice, once in NormalFrameView and once in TenthFrameView, like this:

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

And similarly in the other class. So that removed eight or ten lines of duplication from the two View classes. Can we move more duplication over there? I’d wager that we can. But while that was a useful thing to do, I really just did it to see if it was a good idea. Let’s look at those two classes to see if there is something better to do.

I think that one of the FGNO members, maybe Bill Wake, had used only a single view class with conditional behavior. Let’s look at the two classes that I have and how they differ. First the inits and the draw:

class NormalFrameView:
    def __init__(self, frame_number, frame: Frame):
        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
        self.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.white_lines([(x_right , mark_line), (x_right - mark_line, mark_line), (x_right - mark_line, 0)])
        self.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.draw_first_roll(game)
        self.draw_mark(game)
        ViewHelper().draw_frame_total(game, self.f, self.frame)

    def white_lines(self, lines):
        pygame.draw.lines(game.screen, game.WHITE, False, lines)


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

    def draw(self, game):
        x = 10 * game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.white_lines([(x, 0), (x, game.GAME_Y)])
        self.white_lines([(x, mark_line), (x - 3 * mark_line, mark_line)])
        self.white_lines([(x - mark_line, mark_line), (x - mark_line, 0)])
        self.white_lines([(x - 2 * mark_line, mark_line), (x - 2 * mark_line, 0)])
            self.white_lines([(x - game.FRAME_SIZE, game.GAME_Y), (x, game.GAME_Y)])
            ViewHelper().draw_frame_total(game, self.f, self.frame)
        self.draw_tenth_frame(game, 0)
        self.draw_tenth_frame(game, 1)
        self.draw_tenth_frame(game, 2)

    def white_lines(self, lines):
        pygame.draw.lines(game.screen, game.WHITE, False, lines)

The tenth frame looks just the same as the other nine, except with more little squares. You’d think that the code would look mostly the same as well. If it did, we could exploit that similarity somehow.

Let’s adjust TenthFrameView to make it more like Normal. First, rename x to x_right:

class TenthFrameView:
    def draw(self, game):
        x_right = 10 * game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.white_lines([(x_right, mark_line), (x_right - 3 * mark_line, mark_line)])
        self.white_lines([(x_right - mark_line, mark_line), (x_right - mark_line, 0)])
        self.white_lines([(x_right - 2 * mark_line, mark_line), (x_right - 2 * mark_line, 0)])
        self.white_lines([(x_right - game.FRAME_SIZE, game.GAME_Y), (x_right, game.GAME_Y)])
        ViewHelper().draw_frame_total(game, self.f, self.frame)
        self.draw_tenth_frame(game, 0)
        self.draw_tenth_frame(game, 1)
        self.draw_tenth_frame(game, 2)

That makes the first write_lines call the same in each method. The second write_lines in Normal draws the rightmost small box. Let’s add that to the Tenth and remove the code it uses to draw the vertical part of that box.

I test that on screen, and move a line in Normal and we have this:

class NormalFrameView:
    def draw(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.white_lines([(x_right , mark_line), (x_right - mark_line, mark_line), (x_right - mark_line, 0)])

        self.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        ViewHelper().draw_frame_total(game, self.f, self.frame)
        self.draw_first_roll(game)
        self.draw_mark(game)

class TenthFrameView:
    def draw(self, game):
        x_right = 10 * game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.white_lines([(x_right , mark_line), (x_right - mark_line, mark_line), (x_right - mark_line, 0)])

        self.white_lines([(x_right, mark_line), (x_right - 3 * mark_line, mark_line)])
        self.white_lines([(x_right - 2 * mark_line, mark_line), (x_right - 2 * mark_line, 0)])
        self.white_lines([(x_right - game.FRAME_SIZE, game.GAME_Y), (x_right, game.GAME_Y)])
        ViewHelper().draw_frame_total(game, self.f, self.frame)
        self.draw_tenth_frame(game, 0)
        self.draw_tenth_frame(game, 1)
        self.draw_tenth_frame(game, 2)

Note that I am just more or less blindly trying to make these two methods look alike. I’m not reasoning particularly deeply, just finding similarities in the text and making them more similar.

What is that line below the white space in Normal? It’s the bottom line of the box. We must have similar code in Tenth. Yes, it’s the last line. Add x_left to Tenth, modify the beginning to look more like Normal and use the x_left:

class TenthFrameView:
    def draw(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.white_lines([(x_right , mark_line), (x_right - mark_line, mark_line), (x_right - mark_line, 0)])

        self.white_lines([(x_right, mark_line), (x_right - 3 * mark_line, mark_line)])
        self.white_lines([(x_right - 2 * mark_line, mark_line), (x_right - 2 * mark_line, 0)])
        self.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        ViewHelper().draw_frame_total(game, self.f, self.frame)
        self.draw_tenth_frame(game, 0)
        self.draw_tenth_frame(game, 1)
        self.draw_tenth_frame(game, 2)

Now that last call to white_lines is the same as in Normal, move it up and we have this:

class NormalFrameView:
    def draw(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.white_lines([(x_right , mark_line), (x_right - mark_line, mark_line), (x_right - mark_line, 0)])

        ViewHelper().draw_frame_total(game, self.f, self.frame)
        self.draw_first_roll(game)
        self.draw_mark(game)

class TenthFrameView:(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.white_lines([(x_right , mark_line), (x_right - mark_line, mark_line), (x_right - mark_line, 0)])

        self.white_lines([(x_right, mark_line), (x_right - 3 * mark_line, mark_line)])
        self.white_lines([(x_right - 2 * mark_line, mark_line), (x_right - 2 * mark_line, 0)])
        ViewHelper().draw_frame_total(game, self.f, self.frame)
        self.draw_tenth_frame(game, 0)
        self.draw_tenth_frame(game, 1)
        self.draw_tenth_frame(game, 2)

We could move those three calls to write_lines to the helper and reuse them. We can’t quite move all six duplicate lines because we need x_right in the drawing after the blank line, in Tenth.

In one compartment of my mind, I’m beginning to think about combining the View classes into one and having some kind of strategy object manage the differences. That’s too big a reach for me at this point, and we haven’t even started to work on the rest of the Views’ differences. But I am keeping it in mind.

What does this code do?

        self.white_lines([
            (x_right , mark_line), 
            (x_right - mark_line, mark_line), 
            (x_right - mark_line, 0)])

It is drawing the right-most small box, first the horizontal right to left, then the vertical, bottom to top.

What do these lines in Tenth do?

        self.white_lines([
            (x_right, mark_line), 
            (x_right - 3 * mark_line, mark_line)])
        self.white_lines([
            (x_right - 2 * mark_line, mark_line), 
            (x_right - 2 * mark_line, 0)])

The first line draws all the way across the box at the level of the small boxes. The second draws the vertical line at the 1/3 marker (2/3 from the right), completing the three boxes in the tenth frame.

What if, instead of doing that, we drew two more small boxes? We have one, from x_right going in 1/3. We could draw one from there to 2/3, and then, wastefully, draw one from 2/3 in to the left edge. The upward bar of that one would be redundant, overwriting the long vertical of the ninth frame.

But the code would be interestingly similar.

I’m going to try it. The idea is to make two more calls to write_lines that look similar to the stacked one above. I paste in two copies:

    def draw(self, game):
        x_left = self.f * game.FRAME_SIZE
        x_right = x_left + game.FRAME_SIZE
        mark_line = game.MARK_LINE
        self.white_lines([(x_right, 0), (x_right, game.GAME_Y)])
        self.white_lines([(x_left, game.GAME_Y), (x_right, game.GAME_Y)])
        self.white_lines([
            (x_right , mark_line),
            (x_right - mark_line, mark_line),
            (x_right - mark_line, 0)])
        self.white_lines([
            (x_right , mark_line),
            (x_right - mark_line, mark_line),
            (x_right - mark_line, 0)])
        self.white_lines([
            (x_right , mark_line),
            (x_right - mark_line, mark_line),
            (x_right - mark_line, 0)])

        self.white_lines([(x_right, mark_line), (x_right - 3 * mark_line, mark_line)])
        self.white_lines([(x_right - 2 * mark_line, mark_line), (x_right - 2 * mark_line, 0)])
        ViewHelper().draw_frame_total(game, self.f, self.frame)
        self.draw_tenth_frame(game, 0)
        self.draw_tenth_frame(game, 1)
        self.draw_tenth_frame(game, 2)

Now, using the hints that the two lines below the space suggest, I can edit the two new calls like this:

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

The picture is still good:

picture is still good with three small boxes in tenth frame

N.B.: Actually it’s not quite good. I don’t notice until later. Did you notice?

Now we have the opportunity to create a method, say draw_small_box, on our ViewHelper. It appears that it could use three parameters, x_right, mark_line, and the index of the box to draw.

Let’s see if we can get PyCharm to help with this, at least down to extracting the method into TenthFrameView. We’ll have to move it to the ViewHelper manually, I think.

Extract Variable:

        box_number = 3
        self.white_lines([
            (x_right , mark_line),
            (x_right - box_number * mark_line, mark_line),
            (x_right - box_number * mark_line, 0)])

Extract Method. PyCharm actually notices the other one. Undo, and do this first.

        self.white_lines([
            (x_right , mark_line),
            (x_right - 1 * mark_line, mark_line),
            (x_right - 1 * mark_line, 0)])
        self.white_lines([
            (x_right , mark_line),
            (x_right - 2 * mark_line, mark_line),
            (x_right - 2 * mark_line, 0)])
        box_number = 3
        self.white_lines([
            (x_right , mark_line),
            (x_right - box_number * mark_line, mark_line),
            (x_right - box_number * mark_line, 0)])

Now extract method on the third.

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

    def small_box(self, x_right, mark_line, box_number):
        self.white_lines([
            (x_right, mark_line),
            (x_right - box_number * mark_line, mark_line),
            (x_right - box_number * mark_line, 0)])

Now let’s move the method over to ViewHelper and use it there.

I realize that I should provide the game, although with the helper in the main it happens to compile. So let’s give the Helper the game on creation.

After a bit of fiddling, I’ve moved the two views and the helper off into a file of their own and sorted out the game references. Another thing that I “should” have done sooner, had I not been rushing.

There is good news and not so good news. The two draw methods are now quite similar:

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

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

The bad news is that the picture is no longer quite perfect: there is doubling of the margin line between frames nine and ten.

doubling of margin line between 9 and 10

I think this is most likely a rounding issue, but whatever it is, we’ll have to fix it if we continue with this toy project. I think we can fix it in the small_box method, one way or another. I am still happier with the code, because it has resolved into a small number of methods that we use for similar purposes.

We’ve done enough for the morning, I think. Let’s sum up.

Summary

The biggest concern I have with the code is the graphical bits. It’s not ideal, but now the code that does similar things either calls the same method, or at least looks similar everywhere. We have one sort of “abstraction” that has appeared, the “small box”, and, if it wasn’t clear, the two duplicated calls to white_lines in the two views are both doing “big box”, which could be extracted similarly to what we did this morning.

I do have another concern that is worth mentioning: the frame object has what seem to me to be two distinctive responsibilities: it calculates the frame score details, and it produces various string-formatted information used in the Views. Those two responsibilities might be separated into a Frame and a FrameExplanation object, or some such notion. It might be amusing to try to figure out how best to deal with the raw game facts, which come down to rolls and scores (and possibly strikes and spares), the particular strings we use to communicate with the Views, and the things the Views actually display. Possibly there is a better partitioning of responsibilities to be found.

Will we do that?

I haven’t decided. On the one hand, it might be fun to see how nice we can get this little program to be. On the other hand, we may have wrung out most of the important lessons.

Important lessons?

Good question. I see at least these:

I am susceptible to even self-induced pressure to go too fast. Perhaps a pair would have helped me, but perhaps a pair would have been shouting “Go! Go! Go!”, who knows. I predicted in advance that I’d try to go too fast. I realized as I did it that I was trying to go too fast. And still I didn’t slow down. Target fascination or something.
I probably did less TDD than I could have. Once I got interested in the GUI, it was “easy enough” to test by looking at the GUI. The result of that hasn’t been awful so far, but that’s only because bowling is simple and the rules don’t change. If we had a model that needed change, I think we’d be short of tests and likely to inject defects that we didn’t notice.
Based on the amount of time I’ve spend refactoring and improving the not-very-good code, I was going nearly twice as fast as would have been sustainable. In a real, longer-term product development, that would show up downstream by an apparent slowdown in progress, and, very likely, a surprising uptick in defects.
It is possible to dig back out of the hole with short refactoring sessions, even where things have gotten rather nasty. (I did say at least three!)

Summarizing the summary:

Pressure, even self-induced, will result in a product that looks better than it is, leading downstream to slowdowns and defects.
It is possible to dig back out of the hole, by integrating short refactoring work in with feature work.

We will quite possibly, from time to time, write some crufty code. We can get back to nominal with decent tests and refactoring.

I can’t wait to find out what I do next. See you then! Until then, resist fascism, and take care of one another.