Around Sunday, December 26, 2004, 6:47:11 AM, Victor wrote:
> Very elegant, Alistair!
With the greatest love and respect for both Victor and Alistair, I
must respectfully disagree that the code is elegant. Let's look at
the code with a critical, but, I hope, fair eye.
Remember, we're critiquing this code, not Alistair or Victor.
score: rolls
| score frameStart |
score := 0.
frameStart := 1.
10 timesRepeat: [ | roll1 roll2 open spare strike |
roll1 := rolls at: frameStart.
roll2 := rolls at: frameStart + 1.
open := roll1 + roll2 < 10.
spare := (roll1 < 10) and: (roll1 + roll2 = 10).
strike := roll1 = 10.
score := score + roll1 + roll2.
( spare or: strike )
ifTrue: [ score := score + (rolls at: frameStart + 2)].
( open or: spare ) ifTrue: [ frameStart := frameStart + 2 ].
( strike ) ifTrue: [ frameStart := frameStart + 1 ].
].
^score
What can we see here? Well, we see a method with seven temporary
variables, five of them inside a loop. This is evidence of something
wanting to happen, though it's not clear just what.
Inside the loop, we can identify at least three distinct ideas:
1. Initializing some values that are constant for the rest of the
loop, but (generally) referred to multiple times. Suggestive of an
inner method, perhaps, or premature optimization, but in any case,
one idea: initialize.
2. Calculating the score. This is the point of the loop, yet it
occurs in between two other ideas. Second idea: score.
3. Third idea: Incrementing the frame.
Three ideas in one block of code might be seen as insufficiently
expressive, calling for Extract Method. The large number of temps
might be seen as calling for a new class.
Are we done? Not remotely!(1)
The initialization includes setting up three boolean variables, with
a potential space of 2^3 or eight possible combinations. But the
truth is that exactly one of those booleans will be true, and the
other two false: a space of three possibilities.
In the last two lines of the loop:
( open or: spare ) ifTrue: [ frameStart := frameStart + 2 ].
( strike ) ifTrue: [ frameStart := frameStart + 1 ].
only one of these two conditions can occur. Even if strike could
occur in the presence of open or spare, the strike line would negate
the effect of the first line.
Constructions like ( open or: spare ) cause the compiler to emit
warnings, since the or: method properly takes a block, not a
constant. I'm not even sure that the construct can work as it
stands, or whether it is a fluke of the tests that it appears to.
Even by browsing the code for False>>or: it's not clear what will
happen. I'd need to write a test to be sure.
(I wrote the test and it works. Turns out that Boolean #value
returns self, which makes it work.)
Is that all? I'm just getting started!(1)
The variable names could be better. The booleans might better be
expressed as isStrike rather than strike. Even though I tend to use
the same notation myself, I question it.
The names roll1, roll2 are used, but the obvious roll3 is not. This
is probably some kind of optimization in the first two cases, but
expressiveness might be thought to trump optimization, calling for
creation of all three. And the words themselves are hard to
distinguish, especially the problematical roll1, which is hard to
distinguish from ro11l and ro1l1.
Finally, at least two of the statements include redundant
parentheses.
All this notwithstanding, it is a very interesting implementation:
what it does is rather compact and contains some good ideas.
According to my limited understanding of good, much less elegant
Smalltalk code, however, it could use some work.
I offer these thoughts in the spirit of learning and self
improvement, with the greatest respect for my fellow programmers,
and because it's my birthday.
Ron Jeffries
www.XProgramming.com
(1) Identify that quote.
To Post a message, send it to: [EMAIL PROTECTED]
To Unsubscribe, send a blank message to: [EMAIL PROTECTED]
ad-free courtesy of objectmentor.com
Yahoo! Groups Links
<*> To visit your group on the web, go to:
http://groups.yahoo.com/group/extremeprogramming/
<*> To unsubscribe from this group, send an email to:
[EMAIL PROTECTED]
<*> Your use of Yahoo! Groups is subject to:
http://docs.yahoo.com/info/terms/
