Refactoring example

Smalltalkers,

In recent issues of Software Development magazine there has been an example refactoring
of some Java code.  I converted this to Smalltalk and used it as the basis for a talk
recently.  The refactored code is available at
http://www.ultrasw.com/alcock/tcs/devsig/02jul2002/PrimeGenerator.htm or from a link on
the web page for the talk at http://www.ultrasw.com/alcock/tcs/devsig/02jul2002/ which
includes a link to the original version and to several resources.  Letting other people
see it would probably be a good exercise in humility.  Friendly critique welcome.

Keith Alcock

Keith Alcock wrote:

Keith

John Brant wrote a great post on refactoring a while back, and it made it into Martin
Fowler's book. I can't do that well (I also don't have the time), but I started with your
"refactored" code and worked on that a bit. Here's what I came up with - YMMV.

I find the name <PrimeGenerator> to be more expressive about what this creature actually
is. Implementing the generator with class methods seems to be bad style, so I moved things
over to the instance side. Also, holding the <booleanArray> in an instVar saves us from
having to pass it around as an argument all over the place.

Object subclass: #PrimeGenerator
 instanceVariableNames: 'booleanArray'
 classVariableNames: ''
 poolDictionaries: ''
 classInstanceVariableNames: ''!

!PrimeGenerator class methodsFor!

generatePrimes: anInteger
 ^super new generatePrimes: anInteger! !

Sorry, but I found #generatePrimes: to be just plain ugly. Statements such as

    "self findPrimes: (self siftPrimes: (self initPrimes: maxValue))."

are quite clever (see my Smalltalk Report 12/97 article "The Price of Cleverness" for some
similar examples). I decided to use a guard clause to get out quickly on the boundary
condition. Afterwards, I simply stated the steps to be taken, all at the same level of
abstraction.

!PrimeGenerator methodsFor!

generatePrimes: anInteger
 "Used a guard clause here"

 anInteger < 1 ifTrue: [^Array new].
 ^self
  initializePrimes: anInteger;
  siftPrimes;
  selectPrimes!

The following methods are (hopefully) self-explanatory, although you may prefer a slightly
different mode of expression. I added one additional refactoring, and implemented around
not having access to Keith's extension methods, which means that this code should run in
vanilla Dolphin.

!!PrimeGenerator methodsFor!

initializePrimes: anInteger
 booleanArray := Array new: anInteger.
 booleanArray
  atAllPut: true;
  at: 1 put: false.! !

!PrimeGenerator methodsFor!

removeMultiplesOf: baseIndex
 ^2 * baseIndex to: booleanArray size
  by: baseIndex
  do: [:multipleIndex | booleanArray at: multipleIndex put: false]! !

!PrimeGenerator methodsFor!

selectPrimes
 "#findPrimes: is not as expressive as select, which is what we are actually doing.
 #selectKeys: sounds like a good method name, but I don't have the source"

 | result |
 result := OrderedCollection new.
 1 to: booleanArray size do: [ :index |
  (booleanArray at: index) ifTrue: [result add: index]].
 ^result! !

!PrimeGenerator methodsFor!

siftPrimes
 | maxPrimeFactor |
 maxPrimeFactor := booleanArray size sqrt floor.
 1 to: maxPrimeFactor do: [:baseIndex |
  (booleanArray at: baseIndex) ifTrue: [self removeMultiplesOf: baseIndex]].
 ^booleanArray! !

Cheers

--
Joseph Pelrine [ | ]
MetaProg GmbH
Email: [hidden email]
Web:   http://www.metaprog.com

"If you don't live on the edge, you're taking up too much space" -
Doug Robinson

"Joseph Pelrine" <[hidden email]> wrote in
message news:[hidden email]...
> Keith Alcock wrote:
>
> > In recent issues of Software Development magazine there has been an
example refactoring
> > of some Java code.  I converted this to Smalltalk and used it as the
basis for a talk
> > recently.  The refactored code is available at
> > http://www.ultrasw.com/alcock/tcs/devsig/02jul2002/PrimeGenerator.htm or
from a link on
> > the web page for the talk at
http://www.ultrasw.com/alcock/tcs/devsig/02jul2002/ which
> > includes a link to the original version and to several resources.
Letting other people
> > see it would probably be a good exercise in humility.  Friendly critique
welcome.
>
> John Brant wrote a great post on refactoring a while back, and it made it
into Martin
> Fowler's book. I can't do that well (I also don't have the time), but I
started with your
> "refactored" code and worked on that a bit. Here's what I came up with -
YMMV.

I assume you are talking about my Sort example that I posted to cls a few
years ago. If so, it didn't make it into Martin's book (couldn't fit a
Smalltalk example into a Java book). Anyway, you can find the example on
google:
http://groups.google.com/groups?q=no+comment+sort+group:*smalltalk*+author:b
rant%40cs.uiuc.edu&hl=en&lr=&ie=UTF-8&oe=UTF-8&scoring=d&selm=6q5o1t%24rr8%2
41%40vixen.cso.uiuc.edu&rnum=1

In case this gets sufficiently mangled that you can't follow the link, try
searching google groups with: "no comment sort group:*smalltalk*
author:[hidden email]". It is the one from August 1998.


John Brant

On Sat, 06 Jul 2002 21:41:39 GMT,
John Brant <[hidden email]> wrote:
John,

do you use those extremelyLongVariableNamesConsistingOfManyWords
in real life, too? I noticed that I don't read the whole variable
name, if gets too long, in the above example I would probably use
"extremelyMumble" internally.

I especially liked the following part of your 98 posting:

I then realized that thereal refactoring for this problem is to replace all
code such as"Sort array: aCollection" with "aCollection asSortedCollection" and
remove the Sort class. After this refactoring every line of codeleft doesn't
need commenting -- there are no lines left...

Aaaaah... the ultimate refactoring :-)

s.

"Stefan Schmiedl" <[hidden email]> wrote in message
news:ag7pon$jeq87$[hidden email]...

> do you use those extremelyLongVariableNamesConsistingOfManyWords
> in real life, too? I noticed that I don't read the whole variable
> name, if gets too long, in the above example I would probably use
> "extremelyMumble" internally.

I prefer to use the shortest variable name that conveys the meaning. If this
means having an extremelyLongVariableNameConsistingOfManyWords, then that's
what I will (or should) use. As for reading the variable name, you only need
to read the variable name once and the rest of the time you will do a
pattern match to find the variable (assuming that it is distinct enough from
the other variables).

Anyway, my variable names seem to be somewhat longer than the base
VisualWorks variable names. If I look at all variables (globals, temps, inst
vars, etc.) referenced in my code, I have an average of 9.1 characters per
variable reference and 12.3 characters per distinct variable reference. In
the VW7 base VisualWorks pundle, the average is 8.0 and 11.6 characters
respectively.


John Brant

On Sun, 07 Jul 2002 16:55:50 GMT,
John Brant <[hidden email]> wrote:
>
> Anyway, my variable names seem to be somewhat longer than the base
> VisualWorks variable names. If I look at all variables (globals, temps, inst
> vars, etc.) referenced in my code, I have an average of 9.1 characters per
> variable reference and 12.3 characters per distinct variable reference. In
> the VW7 base VisualWorks pundle, the average is 8.0 and 11.6 characters
> respectively.
>

I just checked a small app that I wrote some time ago.
If I take "distinct variable reference" to mean "counting each
name once, even if it appears in several places", I get
5.6 per reference and 7.2 per distinct ....

Seems to be an eternal burden caused by starting with
BASIC on a ZX81, where it was just too convenient to
type "LET L =", "FOR F", "INPUT K", "NEXT N" because the
key actions were context sensitive.

Feeling old,
s.