Hi,

Thank you for you insightful response. I have added a few comments.

----------
> From: Ralph E. Johnson <johnson at cs.uiuc.edu>
> To: peter at smalltalk.org; squeak at cs.uiuc.edu
> Subject: Re: Temporary variables
> Date: June 18, 1999 8:17 AM
> 
> At 3:43 AM 6/18/99, Peter William Lount wrote:
> >I really think that method size and complexity is a more important issue
> >that the number of temporaries in a method. The names you choose for the
> >variables is also more important than the number of them. Often what a
> >large number of variables illustrates is a potential opportunity to
> >refactor the method. But it may be fine as it is. The number of
temporaries
> >is really a non-issue in comparison.
> >
> >My primary goal is to keep the size of the method down to under 10-20-30
> >lines. I like to spread lines vertically and split expressions onto
their
> >own lines and use temporaries to hold the values of these "split"
> >expressions. It's a form of literate programming where you document the
> >method with temporary variable names as well as comments and good method
> >names.
> 
> I agree.  But if you keep a method down to under 20 lines, you will
> not have a lot of variables.  There will be less than one variable
> per line.

I agree in the "general cases". But there are exceptions and thats were our
points of view differ. In the edge cases I think that the "style rules" or
more apporpiately "style guidelines" are "less important" except as
"indicators".

I can conceive of cases where a calculation method it might be less than 20
lines and have lots of variables.

> 
> >In window creation methods for windows that have lots of screen view
> >components you need to have lots of temporaries for holding the view
> >instances as you create them and then configure them.
> 
> Not usually.  It is good to create components in methods so you can
> override them if you subclass the window.  If you only use a variable
> once, you can replace the use by the definition.
> 
Well yes, usually. Digitalks/Objectshares "Widgets" creates lots of
temporaries when it generates a method. So if you are using their system
you'll get lots of temps as the analysis in the previous email  revealed.
Most of the large methods in the image that I tested in were "window
creation" methods.

> 
> >I just don't like people telling me that "it is required to meet our
style
> >rules". 
I didn't say that you did but I heard it before from others who wish to
enforce their view that it doesn't meet the RULES.

> 
> We didn't say that.  We said "methods with lots of temporaries are ugly
> and should be rewritten."  The argument to this is to give methods with
> lots of temporaries that do not need to be rewritten.
> 
> >I would rather have a large number of temporaries if they claify the
method
> >then having short concise code that is obfucated by just a few variables
> >and lots of expressions in "( )".
> 
> Sure.
> 
> >Often I'll take "sub expressions" and make them their own messages as
well.
> >This can reduce the size of methods and enable reuse of code fragements.
> 
> And reduce the number of temporaries per method.  I do this a lot.
Actually that when I do this the number of temporaries may still increase
if I wish to store the results in a temporary
....
> 
> >Furthermore, this "temporaries for sub expressions" usage allows me to
> >quickly click on the temporary variable in the debugger and see the
value
> >or inspect it without having to "execute the expression" which could
cause
> >a side effect.
> 
> Yes, I've done this, too.  It is a way of getting around a design
> flaw in the debugger.  The debugger should let you see the value
> being returned.
I agree. Lets improve the debugger. It sure needs improving...

> 
> >Often I won't reuse temporaries in the method. I'll just use another
one.
> >Why not? The only reason why not is if the objects being held onto
during
> >the method execution are going to cause memory overflows if your image
is
> >close to it's memory limits. The another only reason why not is if you
hit
> >the limit of temporaries of the Smalltalk your using. But, as long as
your
> >usage of temporaries makes things clearer for you and others who
maintain
> >your code and if it helps in debugging I say go for it as needed. An
> >exception to the rule of keeping your methods short.
> 
> I *never* reuse temporaries!
Great.
> 
> 
> >A nother concern that has not yet been mentioned is that sometimes you
have
> >to write code fast and furoiusly and just get it done. The method could
be
> >refactored to simplify it but TIME AND MONEY don't permit it. Reality of
> >commercial projects.
> 
> I sympathize and mostly agree.  It can take a lot of time to refactor a
> large system, but it doesn't take much time to refactor a single method.
Not true it can take hours if not days to refactor and TEST some of the
calculation methods in the engineering application. I know this from
experience and sometimes the old saying "If it ain't broke don't fix it" is
the best guideline of them all.

> 
> >Some methods by Peter Lount that "violate the style guides"!
> 
> Thanks for placing these out in public view!  It is always hard to
> submit your work to criticism.  I will, naturally, rewrite them.
Well in rewriting them you "broke" the existing "semantics" in the second
rewrite below. 

There are obviously many levels of refactoring: "rewriting without changing
the semantics", "rewriting and changing the semantics", "refactoring the
classes by introducing new type of objects like transformations", etc...
Your first rewrite would work and is a classic example of making new
methods out of existing ones. Fortunately, "I get to choose" which methods
I spend my time rewriting. 

Natually I agree with you that they could be refactored and that a
"transformation" object be used for the transformations. Time was of
importance when I wrote this and the Digitalk Smalltalk does not have such
a beasty. I didn't have time to bring in my "transformation" classes from
another smalltalk into Digitalk and TEST them. Deadlines. Besides which the
method is fine as it is for what is was attempting to do and the context it
was created in.

> 
> >BezierCurve instance method.
> >
> >displayOn: theBitmap 
> >steps: theNumberOfSteps
> >preTranslateBy: thePreTranslatePoint
> >scaleBy: theScalePoint
> >postTranslateBy: thePostTranslatePoint
> >
> >        | aPen aListOfPointsOnBezierCurve aFractionParameter
aPointOnBezierCurve 
> >        aListOfIntegerControlPoints aListOfIntegerPoints |
> >
> >        aListOfPointsOnBezierCurve := OrderedCollection new.
> >
> >        0 to: theNumberOfSteps do: [:anIndex |
> >                aFractionParameter := (anIndex / theNumberOfSteps)
asFloat.
> >                aPointOnBezierCurve := self computePointOnBezierCurveAt:
> >aFractionParameter.
> >                aListOfPointsOnBezierCurve add: aPointOnBezierCurve.
> >        ].
> >
> >        aPen := theBitmap pen.
> >        aListOfIntegerPoints := aListOfPointsOnBezierCurve collect:
[:aPoint |
> >                (((aPoint + thePreTranslatePoint) * theScalePoint)  +
> >thePostTranslatePoint) rounded
> >        ].
> >        
> >        aPen foreColor: Color black.
> >        aPen polyline: aListOfIntegerPoints.
> >
> >        aListOfIntegerControlPoints := self integerControlPoints
collect: [:aPoint
> >|
> >                (((aPoint + thePreTranslatePoint) * theScalePoint) +
> >thePostTranslatePoint) rounded 
> >        ].
> >
> >        aPen foreColor: Color darkRed.
> >        aPen polyline: aListOfIntegerControlPoints.
> >        aListOfIntegerControlPoints do: [:aPoint |
> >                aPen place: aPoint.
> >                aPen circleFilled: 3
> >        ].
> >
> >        ^aListOfPointsOnBezierCurve
> 
> I would write this as
> 
> displayOn: theBitmap 
> steps: theNumberOfSteps
> preTranslateBy: thePreTranslatePoint
> scaleBy: theScalePoint
> postTranslateBy: thePostTranslatePoint
> 
>         | aPen pointsOnBezierCurve integerControlPoints integerPoints |
> 
>         pointsOnBezierCurve := self pointsOnBezierCurveOfSize:
theNumberOfSteps.
>         integerPoints := pointsOnBezierCurve collect: [:aPoint |
>                 ((aPoint + thePreTranslatePoint) * theScalePoint +
thePostTranslatePoint) rounded].
>         
>        integerControlPoints := self integerControlPoints collect:
[:aPoint |
>                 (((aPoint + thePreTranslatePoint) * theScalePoint) +
> thePostTranslatePoint) rounded].
> 
>         aPen := theBitmap pen.
> 
>         aPen foreColor: Color black.
>         aPen polyline: integerPoints.
> 
>         aPen foreColor: Color darkRed.
>         aPen polyline: integerControlPoints.
> 
>         integerControlPoints do: [:aPoint |
>                 aPen place: aPoint.
>                 aPen circleFilled: 3
>         ].
> 
>         ^pointsOnBezierCurve
> 
> pointsOnBezierCurveOfSize: theNumberOfSteps
>         ^(0 to: theNumberOfSteps) collect: [:anIndex |
>                 self computePointOnBezierCurveAt: (anIndex /
theNumberOfSteps) asFloat]
> ====
> I bet you have lots of methods that take thePretranslatePoint,
theScalePoint, and
> thePostTranslatePoint.  If so, I would make a class called Transformation
that bundles
> them up, and change that method to:
> 
> displayOn: theBitmap steps: theNumberOfSteps transformBy: aTransformation
>         | pointsOnBezierCurve integerPoints integerControlPoints aPen  |
> 
>         pointsOnBezierCurve := self pointsOnBezierCurveOfSize:
theNumberOfSteps.
>         integerPoints := aTransformation transformAndRound:
pointsOnBezierCurve.
>         integerControlPoints := aTransformation transformAndRound: self
integerControlPoints.
>         aPen := theBitmap pen.
> 
>         aPen foreColor: Color black.
>         aPen polyline: integerPoints.
> 
>         aPen foreColor: Color darkRed.
>         aPen polyline: integerControlPoints.
> 
>         integerControlPoints do: [:aPoint | aPen place: aPoint;
circleFilled: 3].
> 
>         ^pointsOnBezierCurve
> 
> >=====
> >"Workspace that calculates the temporary variable stats... for
> >VisualSmalltalkEnterprise."
> 
> Well, code in a workspace has entirely different tradeoffs and so
> the usual rules don't count.  For one thing, you can't break it 
> up into methods.  If that code is going to be around a long time,
> I'd make a class out of it.
I agree.
> 
> -Ralph
>