Well, you could try firstRollIndex, at least that conveys that it's an index (although into what?). You could also try firstRollScored, although that's misleading. TBF, I don't think there is a way to concisely state that whole sentence without using words that I'm not familiar with :P

How about ballToScore or scoringBall?

I tend not to worry to much about long names, but I'm sure at some point my "smell detector" would fire. Good luck!

I'd probably wind up with firstFrameBall or firstBallInFrame, since the index of the ball seems to be fairly synonymous with the ball. Just coveting the Test Obsessed bracelet, so thought I'd give it a shot...

The whole thing looks a little painful to me. When I find myself struggling to name a variable or a method I usually end up redesigning the surrounding code so I'm not left with the bad name.

• That's usually what I do too. However in this case the design is just too elegant to pass up. The damned algorith reads like the rules of bowling. I wonder, though, if what I need is some kind of object that hides the that variable. Hmmmmmm..... ==UB

How about simply currentBall, or currentBallIndex? After all, if I have understood correctly, the variable is tracking the index of the ball currently being processed in the loop.

This seems to be an/the 'official' term for a roll (i.e. a quick google of bowling terms, and a browse of the PBA rule book refer to 'shot' in many places).

Looking at the code, it appears to keep track of which shot in the game we are referring to -- in absolute terms (i.e. not by frame). Thus, the number held by the 'shot' variable represents the nth time I actual threw a ball down the lane. "shot += 2" is the same as saying "moving forward two shots brings us to the next frame." Whereas "shot++" says: "moving forward one shot brings us to the next frame." Frames are thus simply ways of, well, framing Shots, rather than being defined by the number of shots in the frame.

Looked at this way, the goal of Bowling could be summarized as: "The winner is the one who gets the most pins in the fewest shots."

Using the taligent naming process.
FirstBallIndex[?] or some variant of that.

So its 'what is' + 'what does'.

You can abbreviate the what does bit, because its going to be a common rule applied to all the code (in your programming style guide). So FirstBallIdx[?], FirstBall[?] (no suffix) or something to make the names short.

But, the real problem is that you have applied meaning to information that really is irrelevant to everyone concerned. Frame is just and index or counter - a junk variable. You only use it once, so why even bother with a name that forces people to read it when its usless info. Use misdirection. ie. 'i' or 'n' or 'pos' would have been good names. People know the syntax of a for() loop, they expect a counter of some kind, so its not important to over-engineer it. Since you've declared the var in the statement its obvious its only got local usage.

Variable names are always subjective to the readers 'myers-briggs' personality type.

Whatever happened to Frame.firstRoll.isStrike() :)

i

The problem is one of context. firstRoll makes sense in the context of a frame, but it makes no sense in the context of an array index unless you're talking about the first item (index 0). In the array context, it's really a frameMarker.
How about the following? It is not quite as clean, but it is expressive. It would also change your logic slightly in the scoring methods.

  public int score() {
   int score = 0;
   int firstRoll = 0;
   for (int frame=0; frame < 10; frame++) {
     if(isStrike(firstRoll))
     {
        score += 10 + twoBallsAfter(firstRoll);
        firstRoll++;
     {
     else
     {
        int secondRoll = ++firstRoll;
        if(isSpare(secondRoll))
        {
           score += 10 + oneBallAfter(secondRoll);
        }
        else
        {
           score += twoBallsInFrame(secondRoll);
        }
        firstRoll++;
     }
   }

Of course, I don't have unit tests, so I don't know if my refactor broke anything!

I'd try frameStart.

James

Are you really happy with a 18-line method that includes three magic 10's and two magic 2's?

• I don't consider the 10 to be magic. The 2's have a bit more magic to them. ==UB

isSpare(firstRoll) confuses the hell out of me - because only the second roll in a frame can turn it into a spare. And then it's the frame itself that's the spare, not the individual roll. Or shot.

If you like this code structure, I'd go for currentShot. But if we were pairing I think I'd be arguing for a Frame class...

• I'd love to see an implementation of the Frame class that doesn't overcomplicate the solution. ==UB

I think this blog really nails the fact that there are at least two different modes that we use when we try to understand code. One is visual/spatial: Being able to see the structure and just get what it is does. The other mode is where we read it like English sentences, using the meaning of the words to put the structure in context. So, yeah, I see where a longer name like indexOfFirstBallInFrame obscures structure a bit but.. me? personally? If I couldn't find anything better, I'd use it over ''firstRoll', because the longer name only obscures slightly and I'd want readers to have another avenue to get their understanding if they can't get what they need from the structure.

For what it's worth, as much as I love good naming, I really love that tight sort of algebraic code that you see in STL implementations also, code with extremely short variable names and all 1-5 line methods. To me, that kind of visual/spatial code is easy to grok but in a different way than English-reading code.

Yeah, I like James' suggestion: frameStart.

There are only three meaningful numbers in software design: zero, one, and many. You've named for "many", but there is only one. Maybe roll or ball or something, maybe. I need to go back to the user story to pick out something a bit better.

Or else it's a matter that you don't have something called "Frame" so that makes it more confusing. I haven't tried this exercise from scratch, but I wondered that there were no Frames in there. I don't know (m)any games so this was my introduction to bowling scores, but I was surprised that the score cards and the description say frames and we don't consider those here.

• Yes, it's remarkable that the algorithm does not mention frames. It make only two concessions to the concept of a frame. The first is the 'magic' 10, and the second is in the way it increments firstRoll. Perhaps there's an elegant solution that involves a FrameIterator[?]? ==UB

"I'd love to see an implementation of the Frame class that doesn't overcomplicate the solution."

Me too. In an effort to find one, I refactored my way to this. I can't say this is any less complicated, but it sure is interesting (well, at least to me...)

public class Game {

    private int currentRoll = 0;
    private int[] rolls = new int[21];

    public void roll(int pins) {
        rolls[currentRoll++] = pins;
    }

    public int getScore() {
        int score = 0;
        Frame frame = firstFrame();
        for (int frameCounter = 0; frameCounter < 10; frameCounter++) {
            if (frame.isStrike()) {
                score += frame.totalPins() + frame.pinsFromNextTwoRolls();
            } else if (frame.isSpare()) {
                score += frame.totalPins() + frame.pinsFromNextRoll();
            } else {
                score += frame.totalPins();
            }
            frame = frame.nextFrame();
        }
        return score;
    }

    private Frame firstFrame() {
        return frameStartingWithRoll(0);
    }

    private Frame frameStartingWithRoll(int roll) {
        return new Frame(rolls, roll);
    }

    private class Frame {
        private int[] rolls;
        private int firstRoll;

        public Frame(int[] rolls, int roll) {
            this.rolls = rolls;
            this.firstRoll = roll;
        }

        public boolean isSpare() {
            return totalPins() == 10 && !isStrike();
        }

        public boolean isStrike() {
            return rolls[firstRoll] == 10;
        }

        public int totalPins() {
            if (isStrike())
                return 10;
            return rolls[firstRoll] + rolls[firstRoll + 1];
        }

        public int nextRoll() {
            if (isStrike())
                return firstRoll + 1;
            return firstRoll + 2;
        }

        public int pinsFromNextRoll() {
            return rolls[nextRoll()];
        }

        public int pinsFromNextTwoRolls() {
            return rolls[nextRoll()] + rolls[nextRoll() + 1];
        }

        public Frame nextFrame()
        {
            return new Frame(rolls, nextRoll());
        }
    }
}

I should see if I can find an old frame based one I did. I remember that I had an 11th frame null object, but aside from that wart it seemed to read pretty well.

I just thought I'd mention it :
Whether you do a strike, a spare, or a simple frame, you ALWAYS add the sum of the pins in the frame to the score. It just itches me when I see a if/elseif/else where an instruction (in this case score += frame.totalPins()) in all of the blocks (that's why the 10 feels like a magic number). As for the variable name, I don't have a clue. I'd go with BradC[?] and try to redesign the surrounding code. Yes, I know it doesn't help. Sorry.

public int getScore() {
	int score = 0;
	Frame frame = firstFrame();
	for (int frameCounter = 0; frameCounter < 10; frameCounter++) {
		score += frame.totalPins();
		if (frame.isStrike()) {
			score += frame.pinsFromNextTwoRolls();
		} else if (frame.isSpare()) {
			score += frame.pinsFromNextRoll();
		}
		frame = frame.nextFrame();
	}
	return score;
}

• Although, on second thought, the initial phrasing looks more like the rules - if the frame is a strike, then the score is all the pins from the frame PLUS the next two rolls, etc - David Chelimsky

Why not leadBall or leadRoll?? Seems appropriate and keeps with the theme. Of course is may not be any better then firstRoll, either. And then leadBall could be mistaken for "led"Ball, as in the metal lead. Hmmm how about x? I give up!

As others have said, the name seems right but it's in the wrong contest. Within a lightweight frame class, it makes more sense. Here's the version I use in C#. The Frame class is private, so there's no worry about "improper" usage, and it's a flyweight, so I only have one of them.

	public class Game
	{
		private int[] rolls = new int[21];
		private int currentRoll = 0;

		public void Roll(int pins)
		{
			rolls[currentRoll++] = pins;
		}

		public int Score()
		{
			int score = 0;
			int frameCount = 10;
			Frame frame = new Frame( rolls ) ;
			while ( --frameCount >= 0  )
			{
				score += frame.Score;
				frame.Advance();
			}
			return score;
		}

		private class Frame
		{
			private int[] rolls;
			private int firstRoll = 0;

			public int Shots
			{
				get { return IsStrike ? 1 : 2; }
			}

			public Frame( int[] rolls )
			{
				this.rolls = rolls;
			}

			private bool IsStrike
			{
				get { return rolls[firstRoll] == 10; }
			}

			private bool IsSpare
			{
				get { return rolls[firstRoll] + rolls[firstRoll+1] == 10; }
			}

			public int Score
			{
				get
				{
					int score = IsStrike ? 10 + StrikeBonus()
						:  IsSpare ? 10 + SpareBonus()
						:  SumOfPins();
					return score;
				}
			}

			private int StrikeBonus()
			{
				return rolls[firstRoll+1] + rolls[firstRoll+2];
			}

			private int SpareBonus()
			{
				return rolls[firstRoll+2];
			}

			private int SumOfPins()
			{
				return rolls[firstRoll] + rolls[firstRoll+1];
			}

			public void Advance()
			{
				this.firstRoll += this.Shots;
			}
		}

• I like this solution. There is almost a direct mapping back to the orginal; and yet it's been nicely repartitioned so that the concept of Frame is not hidden the way it is in the original. The only quibbles I have are that it's a bit less direct, and that the class Frame might be better named FrameIterator[?]. == UB

public boolean isSpare() {
return totalPins() == 10 && !isStrike();
}
Whats with this stuff.

One of its possibilities translates to return 1 && 0. Whats the intent behind that ?

• It's pretty standard idiomatic C. The function returns a boolean, true or false. In this case it returns true when the total of the frame is 10, but the frame is not a strike. That sounds right to me. == UB

firstRoll says where to look for the first shot in whatever frame we are in. My suggestion:

firstRollLocation

I am working on a solution to the variable naming problem, tests and all. However, I just ran into something that I thought was odd. Why didn't you just perform a rollMany(2, 5) to obtain your spare instead of creating a separate method in the test fixture - rollSpare? Just a thought...

-Brian

I suggest changing 'firstBall' to 'currentRoll' and then renaming 'twoBallsForStrike' to 'twoBallsFromStrike' so that it reads 'twoBallsFromStrike(currentRoll)'. This is because you are scoring the next two balls following the strike and moving forward.

Likewise, you would rename 'ballForSpare' to 'ballFromSpare', and 'twoBallsInFrame' to 'ballFromRoll'. The code would look like...

<pre>
public int Score()
{
int score = 0;
int currentRoll = 0;

for( int frame = 0; frame < 10; ++frame)
{
score += thisRoll(currentRoll);

if ( isStrike(currentRoll) )
{
score += 10 + twoBallsFromStrike(currentRoll);
currentRoll++;
}
else
{
if( isSpare(currentRoll) )
score += 10 + oneBallFromSpare(currentRoll);
else
score += oneBallFromRoll(currentRoll);

currentRoll += 2;
}
}

return score;
}
</pre>

-Brian

Follow the rules of TDD...
Test before posting :)

edited code

public int Score()
{
int score = 0;
int currentRoll = 0;

for( int frame = 0; frame < 10; ++frame)
{
score += thisRoll(currentRoll);

if ( isStrike(currentRoll) )
{
score += twoBallsFromStrike(currentRoll);
currentRoll++;
}
else
{
score += oneBallFromRoll(currentRoll);

if( isSpare(currentRoll) )
score += oneBallFromSpare(currentRoll);

currentRoll += 2;
}
}

return score;
}

My brain-wave: frameOrigin

You're welcome.

It's really firstRollOfCurrentFrame, and when I see "of" I think I need a little class. xOfY really means property x of class Y.

In each body of the if statement, you do two things: (1) tally the score for the frame, (2) advance to the next frame. If you extracted these two things to a new little class, like a FrameIterator[?], you could reduce the code to this:

public int score() {
    int score = 0;
    Frame frame = new Frame(rolls); // I assume this is your array of rolls
    for (int frame = 0; frame < 10; frame++) {
        score += frame.score();
        frame.advance();
    }
}

Now, in Frame, the name firstRoll has adequate context to be read as "first roll of current frame". If you prefer the name FrameIterator[?], be my guest. I'm not married to it.

class Frame {
    private int firstRoll;

    // isStrike(), isSpare(), twoBallsForStrike(), ballForSpare(), twoBallsInFrame()
    // all use firstRoll and so don't need it passed in as a parameter any longer

    public int scoreCurrentFrame() {
        if (isStrike())
            return 10 + twoBallsForStrike();
        else if (isSpare())
            return 10 + ballForSpare();
        else
            return twoBallsInFrame();
    }

    public void advance() {
        firstRoll += (isStrike() ? 1 : 2);
    }
}

You could also do "score then advance", but that violates Command/Query separation, so I'm not sure it's better.

Pretend I didn't see that Charlie had already suggested the exact same thing. Great minds think alike.

'bidx' which stands for ball_index. like this:

int bidx = 0; // ball index

imho short names are better since they are easier/faster to read.
they make the code shorter and imho more precise - when them the code looks like a sequence of math formulas which is a 'good thing' in programming. i usually add comments to clarify abbreviations...

i should have read all comments before posting.. really nice solutions are given there :)

I was quite taken with Charlie Poole's and J.B. Rainsberger's two-object approach. It seemed to resolve the variable name issue quite nicely, by eliminating the variable altogether. But upon closer scrutiny the approach doesn't really do that. The variable still exists, and it's name is still troublesome. What the two-object approach really manages to do is separate a for loop from two if statements, at the expense of a new class, and a bit of glue code. In the end, I don't think it's worth it; especially since it doesn't really solve the variable naming problem.

Sorry UB. I watch the tube with closed-captioning turned on. (-;

Maybe i'm reading the intention wrong.. but isn't this a sequence of rolls.

if this roll is a strike,
else if this roll is aSpare,
so that would make me think of it as "thisRoll", the complication i see is how the variable
is used with twoBallsForStrike , ballForSpare, twoBallsInFrame.. .. it might help to clarify
if those methods were actually

twoBallsForStrikeFrom(thisRoll);
ballForSpareFrom(thisRoll),
twoBallsInFrameFrom(thisRoll).

This makes alot of sense to me. but i may be missing some context regarding the algorithum or the domain.

Cheers,

thisRoll, myRoll, theRoll, aRoll, ballRoll.... Perhaps, if it's a pointer to a list of rolls, then, theseRolls, myRolls, theRolls, ballRolls. Lots of overthinking gets silly, huh? Yet, I suppose one ought to try very hard to be clear.

firstRoll is definitely a bad variable name...especially after it is incremented and such...what is it then? 2nd or third roll? :D

Seriously, ballThrown is probably the better choice. However, I tend to think that there is a design problem whenever it is really hard to come up with a meaningful and appropriate variable name.

If we look at the implementation of "score()" we are really talking about a "pull" design that performs the sequence of operations related to bowling. There is no "bowling" game instance as such, rather there is only a call made to score that goes out and gets 10 frames worth of data through operations we can imagine and are noted here but not defined here. One could as easily replace "score" with "main" for all the good "score" provides as it relates to anything bowling but the outcome of the sequence events it uses. There are not even any "try/catch"s so we must believe that each operation called inside score is going to manage the contract per score's needs. That kind of design suggests building "unwholesome" dependencies.

While the algorithm and simplicity of producing an accurate result are perhaps enviable in this design, we haven't done anything that models a real bowling game in any kind of way. Rather, we've "short-handed" the "functionality" of bowling to its I/O necessary for producing a score, which is really all that a computer can really do when it comes to bowling anyway, right?

Bowling is "event driven." We wait our turn, take a ball, enter a lane, throw the ball, score the pins and use decision support logic to determine our next steps. If we throw a count of 10 we sit down, if we throw less than ten we typically throw another ball (disregarding various possible rules for foul handling, etc.) Unless it is the 10th frame we sit down after the second ball in any other frame no matter what. I'm sure that everyone already knows the game well enough, so I won't dwell on its play, but I believe that the design of the program should also model "event infrastructure" of the real-world object where appropriate and where possible.

The above design seems to plan for the condition "else if( (isSparefirstRoll) )" to fail every time that a first ball is not a strike. I don't know about your game, but with mine, I'd be hitting that breakpoint quite often!

While the implemented algorithm is "succinct" and, with good reading of the code and a thorough knowledge of the game...is easily figured out what it does, that doesn't make it a good implementation choice, IMO. Perhaps in a "bowling server" where millions of online bowlers were playing, it would be an optimization necessity, but I'd think that a better design would more closely parallel the game itself so that the "events" encountered in a game of bowling would be realized in the code. Apparently the implementation of isSpare (returns bool) is responsible for getting the I/O necessary to complete the concept of whether the value of the "firstRoll" and whatever isSpace does with it is equal to something that isSpare decides is a spare.

I guess that my point here is that by just a casual look at "score," its implementation (though obvious from the position of knowing bowling scoring) presents more questions than it answers.

It is also a bit difficult to understand how the "nuance" of the 10th frame is handled by looking at it as "frame" is a local it would suggest that none of the operations used inside of score have any concept of a frame. In fact, the names of ballForSpare and twoBallsInFrame would leave a casual observer with the question of "what happened to threeBallsInFrame when frame = 9 (tenth)?" By circumventing the "natural order of events" of what happens in a game of bowling, the implementation becomes less truly understandable...even though we know that the order of events must happen appropriately in order to produce a valid score.

And, since reuse is another topic found elsewhere among these pages, the implementation of score is stateful-dependent. In other words, since it calls operations that get the ball "data" in "real time," we can not even feed score a stream of frame data and have it provide the intended work. Obviously, we'd look for a score implementation that had an appropriate argument list for expectation of that desired outcome, but then, we'd have two different but similar implementations in code of a bowling game scoring system that could not leverage each other's algorithm...thusly preventing any possibility of reuse at the time of implementation.

Some years ago I read an article (online or print, can't remember) where you and (Kent Beck?) were demonstrating XP's pair programming by writing a Java-based implementation of a bowling game. The article said that it took the two of you about an hour...suggesting that you also paid full attention to XP's methodology. So, I wondered how long it would take to hack out a reciprocal bowling "system," but I chose C instead, intentionally adding it as a form of amusement to otherwise entertain me on a 3 hour coach-fare, middle-seat flight. It took me about two hours, counting drinks and latrine interrupts for window seat and myself...and, the natural pain that comes with mind-shifting back to C when C++/Java and other OOPL "comforts" are discarded. It was simply a way to pass the time on a flight.

While I believe that the implementation above would likely work suitably for C, C++ or Java, it appears to me to be more evolved (refactored?) than a test-case first, code to pass the test implementation would suggest. In my mind, the solving of the problems at hand in implementing such a system are more granularly attacked in traditional divide and conquer fashion.

It seems to me that a more obvious and understandable implementation would work against a known set of frame data rather than an implementation where the meaning of "frame" is nothing more than an insurance policy to keep us from looping more than 10 times.

Rob, can you post the C version?

I don't know if I even still have the code. I wrote it some years ago. Basically, the design was based on a Game structure containing an array of Player structures that contained an array of Frame structures. The Player contained a member "extra_ball," which is how I handled the 10th frame without having to have any special Frame for the 10th and so that I didn't have to carry around space for 3 balls in frames 1-9. Nearly all of the processing was done by passing the pointer to a Player around to function pointer members of the Game structure.

I had functions like "throw_ball( Player* player )" that would access the player's members. There are a lot of design issues that would be much improved upon if written in C++, but I felt that allocating storage for the data was a very important aspect of the design.

In the above Java code, the "score" operation doesn't work on any real data set, just the return values for those operations that it calls...and it seems that it would do so in "real time." If we march through the code of "score" from the top, we are always calling "isStrike( 0 )" based on the value of firstRoll due to its initialization...at least for the first pass. The idea that score works on data by modifying an index (presumedly into the data) "position" of data that it doesn't own, can't see and is assumedly hidden by the abstraction of the interface "isStrike" seems to create a dependency that is a "bit weird." However, since we know that "isStrike" returns a boolean, and firstRoll is an index into the data, we're making score dependent on knowing the organization of the data and isStrike dependent on a position of data that we can logically assume that it doesn't "own" in any particular sense. If we assume that all of these operations are some members of a higher-level class (necessitated by Java), what kind of class are we defining? It isn't a BowlingFrame[?] class, since the BowlingFrame[?] would simply be able to score its own count of pins, yet "isStrike" and "isSpare" are operations that have some linkage to BowlingFrame[?] both in terms of scoring the game and in terms of how play progresses. For example, in frames 1-9 any strike can only be on the first ball, and, if it occurs, only one ball is thrown for that frame. These important "elements" seem to belong to BowlingFrame[?] and not really BowlingGame[?]. However, "score" seems to most likely want to belong to BowlingGame[?]. This suggests that BowlingFrame[?] can hardly have a score operation, but must somehow expose isStrike, isSpare, getPinCount.

The implementation of the rules for the game and the management responsibility of conducting play seem to belong to BowlingGame[?].

If we implement a C++ or Java BowlingFrame[?], it is very easy to add a vector of PinsPerBallThrown[?] or some other such concept so that the 10th frame can easily contain 3 balls if necessary and BowlingFrame.[?]getPinCount() will simply be an implementation of looping through the elements of the vector<int> whilst summing them. Therein, BowlingGame[?] can apply the rules of the game while BowlingFrame[?] happily stores the data and validates it on input such as may be exposed through BowlingFrame.[?]insertPinsFelledByBall( int pins, int ball ) ...BowlingFrame[?] will know that pins can not exceed 10 and ball can not exceed 3.

Additionally, BowlingGame[?] can organize the BowlingFrames[?] such that it has 10 of them and BowlingGame[?]'s score operation loops through the container of BowlingFrames[?] calling isStrike? isSpare? getPinCount to sum the score. What should a BowlingFrame[?] know about the game's score?

I tend to start the "BowlingKata[?]" from a visualization of the game from start to finish, excluding the smelly shoe rental :) I picture a score sheet and players. I see an ordering of players names being written into a slot next to their frame data container. I see the external processor adding pin counts to frames (BowlingFrame[?] can easily expose setStrike and setSpare) and then processing a score based on the state of play and a query of frame data. I also see this governor saying, hey Uncle Bob, 10th frame, throw another ball. It seems that the code above directs activity based on its powerful knowledge of what to do in the 10th frame if the first two balls are either a strike or spare. The idea that score somehow makes something happen in the game seems "bass ackwards." :D

P.S. This UI for adding comments seems to lack an ability to format responses and/or even preview and/or reasonably proofread before submitting. Perhaps another UI is in order? It would be great if a phpBB kind of forum were possible. Perhaps it could be configured so that the "bloggers" can only initiate threads, but others can comment/follow-up on them? It doesn't seem like it would be too great of a hack to make it work.

Take Care.

Rob!