Some discussion of style triggered by reader input. Then on to another "improvement", followed by discussion of whether it's an improvement, and why we did it.

Matters of Style

As I mentioned in the updated first article in this Smalltalk Bowling series, Doug Swartz wrote to me, mentioning among other things, the proper use of “==” and “=”. I was using them improperly, though in a way that happens to work, owing to excessive use of C#. No excuse, sir. Doug also raised another issue:

In article two, I would never have left the #frames method as:
 
          ^(1 to: 10) collect: [ :frameIndex | self frameArray ]
As far as I'm concerned, it's a code smell to use the block with an argument, but then ignore the argument. Although it's more lines of code, it feels more intention revealing to do:
            |frames|
            frames := OrderedCollection new.
            10 timesRepeat: [frames add: self frameArray].
            ^frames
Yours is certainly less code, though, and probably runs marginally faster because I didn't pre-size my OrderedCollection. Actually, as I read article 4, it seems you prefer a style which minimizes or eliminates temp variables (ie. more discussion of frames method refactoring). Each to his own, I guess.

When it comes to coding standards, local rules prevail. I do suspect that I use single-expression closed-form collection expressions more than the average bear. I characterize these as “idiomatic Smalltalk”, but it’s fair to mention two things in Doug’s favor. First, many very experienced Smalltalkers, like Doug, share his preference. Second, the closed-form expressions are sometimes harder for new Smalltalkers to understand. Third (I lied about the two), the unused block argument makes the code less clear, and I should at least have said :ignored.

My preference for the closed form expressions could be a vestige of my math background, or of my work in Lisp and other expression-oriented languages, where ugly side effects like storing a temp variable are avoided wherever possible. Or, I might just be overly enamored of the collection protocol. Either way, tastes vary and it’s well worth mentioning.

In the same note, Doug mentioned that he liked the refactoring in the preceding article, because it was bothering him that the scoring algorithm consumed the rolls. He felt that it should be possible to send #score to the same game twice and get the same answer. Good point. I felt a little of that too, but was probably overly proud of the clever way I avoided an index by eating the list of rolls. Had I needed to make #score work repeatedly, I’d have been tempted to do so by copying the rolls collection before eating it. That would have worked, and been easy, but would have been more processing and more obscure. I think the current scheme where everyone gets the same rolls collection is definitely better. And Doug seems to agree, so that makes it official: this is definitely another way of doing it.

Reasonable minds, or mine and Doug’s, may disagree on what’s better. When you’re pairing or working in a team, and when you’re working alone, you need to get a sense of “good enough”, or you’ll tweak the code forever, or loop with two programmers changing something back and forth. Some people worry about that looping, but I’ve never seen it happen very long. Pretty soon someone yells “Who keeps changing my fubar algorithm!?!?”, there’s a discussion in the parking lot, and then the code goes stable. Usually without even creating an opening for a new programmer. Reasonable minds will converge, especially when there’s real work to be done.

We don’t have that problem here: there is no real work to be done, and I intend to explore some of those ideas I mentioned next time, starting now. Partly I’m doing it because I want to draw out a point of difference between Smalltalk and other languages. Partly because I want to practice. And partly, just because I can. But hang in: I have the feeling something interesting will happen.

Potential Improvements

Last time, I listed these possibilities, of which we have now done the first:

  1. My thought last night was this: the Frame objects are each being handed their own little array of just the rolls they should consider. That determination is being made in the BowlingGame, not in the Frame. So ... what if each Frame was instead handed all the rolls, plus an integer telling the Frame where its first ball is?
  2. What if we build the Frames in BowlingGame, but we linked them together instead of putting them in an array. Instead of adding up their scores, we could send #gameScore to the first one, and he would answer self score + next score. That might be pretty nifty.
  3. We might build the Frames one at a time. What if the BowlingGame just created one Frame at the beginning, and as rolls came in, sent them all to it. That Frame would check if it had enough rolls to do its job, and if so, send the rest to its next Frame (creating it if need be). The rolls would ripple down, creating new frames as they went. Then we'd do #score as above.
  4. Or, how about this? If we had it set up like above, then Frames could know their score as soon as they had all the rolls they needed. What if, instead of saving the result, or returning it on request, they sent it back to the BowlingGame? The BowlingGame would then have a running total, which would be correct at the end of the game. And each Frame would know its score as soon as possible, which is interesting in light of the fact that people often ask what we'd do if we had to do incremental frame-by-frame scoring.

Looking at the code we have now, there’s not much incentive to improve it. It’s pretty clean. There is this method, however, of which I might be inordinately proud:

frames
  ^(2 to: 10)
    inject: (OrderedCollection with: self firstFrame)
    into: [ :frames :ignored | frames add: (Frame fromFrame: frames last); yourself ]

That’s a bit tricky. We could expand it in the style Doug suggested, get rid of the #yourself, not use that cute summing trick that I’m so fond of, and make it larger but perhaps not really more clear. Relating to this method, we also have this:

score
  ^self frames 
    inject: 0
    into: [ :sum :each | sum + each score ].

which computes the game score. (Should we call that method #gameScore? Maybe. But we’re here to drain the swamp, not kill some baby alligator.) Let’s think about the swamp:

The game makes a collection of rolls. It then parses those rolls into a collection of Frames. Then it loops over the Frames, asking them for their score, and adds it up. Now that’s not too awful, but there’s kind of a two-phase aspect to the BowlingGame. First rolls, then Frames. And the Frames, though they are more like part of the game, go away, while the rolls, which really only get glommed together in our program, never in a real game, persist.

Now, for consistency with my other examples, I intentionally started with a series of rolls in the tests. Working this way, in C# or Java, I almost never get to a solution with Frames. I usually wind up with a rather procedural solution that is neat enough and compact enough to be a credible stopping point, especially for an hour-long demo. (For an example and some discussion, see The Bowling Game. It will also give you an enlightening comparison of the size difference between essentially the same solution in C# and in Smalltalk.)

Working in Smalltalk, my sense of style really pushed me toward the Frame solution. Now I might have a warped C# style, owing to a C and C++ background, but I see the same thing in lots of client code. C# and Java solutions tend to be more procedural than Smalltalk ones. I think the reason is that the code is less malleable, so that one stops sooner.

Today, though, we’re going to stop later, perhaps later than we should. Let’s let ourselves feel a little concern over the fact that the solution we have now preserves rolls, which are not natural to the real game of bowling, and loses the Frames, which are. And we’ll let ourselves feel a little concern over the slightly greater complexity of those two methods, and the two pass algorithm. Think of it as sanding our fingertips at home before opening that important vault at the casino. Preparation and practice.

What To Do?

The list above contemplated some related ideas: Build the frames as a list and just ask the first one for the game score, letting it figure it out with some help from its friends; build the frames as the rolls occur; do scoring incrementally instead of at the end. This last one gives us some rationale for going forward. A real bowling scoring program might well need to compute the score of each Frame, and it might even need to do it incrementally, since that’s how you do it on paper. So if “for the heck of it” isn’t working for you, pretend we have a story for that.

Our target then, as we understand it, is to wind up with Frames, not rolls, in the BowlingGame, and to have the Frame scores come into being as soon as possible. We’re going to preserve our interface, however: the rolls still come in undifferentiated, one after another, with no indication of which Frame they belong to.

I have no pair here to discuss this with (and at 4 AM, glad of it) so there might be something simpler, but what I think I’d like to start toward is this: Change the BowlingGame so that it just has a frameList instance variable, which will contain a Frame, which will link to other Frames, until done. Then get the score by sending a message to the head of the list, letting him sort out the rest. That’s probably too much to do in one bite, so we’ll have to think of a simple start. (We could just start over and create a new BowlingGame and go from there, but that smacks of “big refactoring story”, and since I won’t let teams get away with that, I’d better not do it here, even though this will only take an hour or so, not days.) So … how to start?

Make the Frames a List

How about this? We’ll make the Frames into a list, and then implement a gameScore method on Frame. We should be able to do that pretty quickly, without too much trouble. First, I’ll make the #frames method link the frames together while it builds the collection. To do that, I’ll add an instance variable, next, to Frame, then set it in the #frames method:

frames
  ^(2 to: 10)
    inject: (OrderedCollection with: self firstFrame)
    into: [ :frames :ignored | frames add: (Frame fromFrame: frames last); yourself ]

Now I can do this all inside the inject, I think, but this is an example of a place where Doug’s preferred style might have been better, by being more procedural. I’m tempted to do it in line, but let’s unwind the method first – and test it:

frames
  | frames |
  frames := OrderedCollection new.
  frames add: self firstFrame.
  9 timesRepeat: [
    | newFrame |
    newFrame := Frame fromFrame: frames last.
    frames last next: newFrame.
    frames add: newFrame].
  ^frames

Now then: the current tests still run, but we just wrote some code that wasn’t driven by a test. That linking stuff isn’t being checked. I believe that the frames are now linked together, but I have no real way of knowing that. Bad Ron. But what could we test. My next step will be to change the way the actual score method works, sending a message to the first frame in the list. That should serve to show that this works. The linking is a first step in that, and I feel confident that it works. If I had put it off, I feel that I’d have had a bigger bite. Maybe I need a pair after all, but for now I’ll press on. Let’s change score and see what happens. It starts like this:

score
  ^self frames 
    inject: 0
    into: [ :sum :each | sum + each score ].

And I’ll make it do this instead:

score
  ^self frames first totalScore

Right. Just send #totalScore to the first frame in the collection. This of course fails utterly, since totalScore isn’t implemented on Frame. We’ll fix that:

totalScore
  ^self score + self nextTotalScore

nextTotalScore
  ^next isNil
    ifTrue: [0]
    ifFalse: [next totalScore]

Yep, that’s it. All the tests run. We just removed the iteration from BowlingGame’s score method. Now let’s remove the collection, by making frames return the list head (i.e. the first Frame):

frames
  | frames |
  frames := OrderedCollection new.
  frames add: self firstFrame.
  9 timesRepeat: [
    | newFrame |
    newFrame := Frame fromFrame: frames last.
    frames last next: newFrame.
    frames add: newFrame].
  ^frames first  

score
  ^self frames totalScore

We begin by just returning the first Frame from #frames, and by not sending first to the collection in #score, since we don’t have a collection any more. We might consider renaming the method #frames to #frameList or even #firstFrame. But I’m not sure that #frames is bad, since we are returning a list. We’ll see. First, let’s simplify the #frames method:

frames
  | firstFrame previousFrame |
  firstFrame := self firstFrame.
  previousFrame := firstFrame.
  9 timesRepeat: [
    | newFrame |
    newFrame := Frame fromFrame: previousFrame.
    previousFrame next: newFrame.
    previousFrame := newFrame].
  ^firstFrame

Having written this fairly reasonable list code, I notice something I should have noticed the first time. We have enough information in the #fromFrame: method to link up the list at that point. The method won’t be just a constructor any more, but let’s worry about that in a moment. First:

frames
  | firstFrame previousFrame |
  firstFrame := self firstFrame.
  previousFrame := firstFrame.
  9 timesRepeat: [ previousFrame := Frame fromFrame: previousFrame.].
  ^firstFrame

Frame class>>fromFrame: aFrame
  | newFrame |
  newFrame := self
    rolls: aFrame rolls
    startAt: aFrame nextFrameStart.
  aFrame next: newFrame.
  ^newFrame

Now this has simplified the BowlingGame’s job a bit, but now we have some odd complexity in the Frame #fromFrame: method, which is now a constructor but also links what it builds to its input. We could make it more obvious what we’re doing with a name change, but if we’re doing lists, let’s do lists. Hold on, I think this is going to get weird:

frames
  | firstFrame |
  firstFrame := self firstFrame.
  9 timesRepeat: [ firstFrame addFrame.].
  ^firstFrame

We’re just going to talk to the firstFrame in its role as head of the list, and tell it nine times to add a Frame. Since all the inputs to the new Frame come from the previous Frame anyway, we can do that. But keep holding on. We have to implement #addFrame on Frame now:

addFrame
  "add a new frame to the end of the list"
  next isNil
    ifTrue: [ self addFrameHere ]
    ifFalse: [ next addFrame ]

Hmm, OK. If we have no next, we’ll add a Frame here. If we already have one, then it’s the next guy’s problem:

addFrameHere
  next := Frame fromFrame: self

We’re done: the tests run. One last thing: now I can revert the #fromFrame: method not to do that ugly linking trick:

fromFrame: aFrame
  ^self
    rolls: aFrame rolls
    startAt: aFrame nextFrameStart

And the tests run!!

Reflection

We have reached that place that Alistair complained about in the previous article: the methods just call other methods, no one does anything, and at the bottom, there’s nothing. Let’s review all the code again:

BowlingGame
-----------
initialize
  rolls := OrderedCollection new.

roll: anInteger
  rolls add: anInteger

score
  ^self frames totalScore

frames
  | firstFrame |
  firstFrame := self firstFrame.
  9 timesRepeat: [ firstFrame addFrame ].
  ^firstFrame

firstFrame
  ^Frame rolls: rolls startAt: 1

Frame
-----
class>>fromFrame: aFrame
  ^self
    rolls: aFrame rolls
    startAt: aFrame nextFrameStart

class>>rolls: aCollection startAt: anInteger
  ^self new
    setRolls: aCollection
    startAt: anInteger

setRolls: aCollection startAt: anInteger
  rolls := aCollection.
  startAt := anInteger

addFrame
  next isNil
    ifTrue: [ self addFrameHere ]
    ifFalse: [ next addFrame ]

addFrameHere
  next := Frame fromFrame: self

nextFrameStart
  ^startAt + self frameStep

frameStep
  ^self isStrike
    ifTrue: [1]
    ifFalse: [ 2]

totalScore
  ^self score + self nextTotalScore

score
  self isStrike ifTrue: [ ^10 + self secondRoll + self thirdRoll].
  self isSpare ifTrue: [ ^10 + self thirdRoll].
  ^self firstRoll + self secondRoll.

isSpare
  ^self firstRoll + self secondRoll = 10

isStrike
  ^self firstRoll = 10

nextTotalScore
  ^next isNil
    ifTrue: [0]
    ifFalse: [next totalScore]

firstRoll
  ^rolls at: startAt

secondRoll
  ^rolls at: startAt + 1

thirdRoll
  ^rolls at: startAt + 2

rolls
  ^rolls

Still reflecting, there are fourteen methods in the Frame instance, and that looks like a lot. Many of them are simple accessors and explaining methods like firstRoll, but still, we might be worried. VisualWorks Smalltalk, like most Smalltalks, includes method categories that you can use to classify methods into groups for easier browsing. So we might be OK with this situation. But is there another way to look at it?

The Frame object now has two kinds of behaviors. One is that it behaves as a list. It has links and methods that it uses to move up and down the list, creating itself or scanning itself to compute the score. It also has single-frame scoring methods, #score, #isSpare, #isStrike, and the roll helpers. So this object isn’t entirely cohesive, embodying single-frame behavior and list behavior in one class. This is a bit of a code smell, and we should at least consider doing something about it. There might be an object here, trying to be born. Maybe the new one is FrameListElement, which has a Frame, and a next. That would push all the frame scoring methods into Frame, and put all the list building and scannign methods in FrameListElement. Perhaps half a dozen each. Would it be better, or worse? Two very small classes, or one class with fourteen methods? I’m not sure at this point, and we still have a couple of ideas left. But first …

Deeper Reflection

I’m not sure whether you can get the feeling, with all the explaining that’s going on here. But these changes were easy. I would never have done anything like this in C# or Java: it would have taken too long, I would have lost the thread of ideas, I would have had to redeclare variables all over.

Whether we like this place or not isn’t the real message, at least to anyone who has the ability to use Smalltalk. The real message is that the language and tools we use influence how we think and work. Using this system, I’m enabled to adjust and modify my work faster, and in finer detail than I can in other systems. Even if we’re stuck in C# or Java, or even C++, our tools and the way we use them will impact the fineness of the work we can do. Tools as simple as a good make or ant build, tools like our friendly neighborhood xUnit, tools for refactoring. Emacs, even. If we have to do everything in a flat editor with no power tools, we just won’t have the time, energy, or really the ability to craft things so finely.

Whether we use the ability to craft good things, or to over-polish, is still a matter of judgment. The point is, the tools, and our skill with them, do make a difference. If we can’t use Smalltalk, maybe we can use Ruby. If we can’t use Ruby, maybe we can beef up our toolkit in some other ways. Our ability to do things will be improved, and we’ll have a chance to move toward better designs that may be eluding us now because of the difficulty of making change.

So if you can’t move to Smalltalk, let the ease of doing things in this language and system inspire you to improve your own workshop. In aid of that, next time we’ll push this design even further. Let’s open her up and see what she can do!