Happy birthday, Ron!
I like this conversation, Alistair, Ron, and everybody else. :-)
After reading Ron's comments, I'll change mine to "very elegant progress,
Alistair".
Because we are in an XP environment, which is by its own nature
evolutionary, supportive, and contributory, I can only see in a positive
light any comments that improve the quality of a work product.
> According to my limited understanding of good, much less elegant
> Smalltalk code, however, it could use some work.
How would the code look after your suggestions have been taken into account?
Victor
======================
----- Original Message -----
From: "Ron Jeffries" <[EMAIL PROTECTED]>
To: <[EMAIL PROTECTED]>
Sent: Sunday, December 26, 2004 9:54 AM
Subject: Re: [XP] New Article - BowlingForSmalltalkV - a small procedural
example
>
> 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 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/
