Temporary variables

Ralph E. Johnson johnson at cs.uiuc.edu
Fri Jun 18 15:17:57 UTC 1999


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.

>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.


>I just don't like people telling me that "it is required to meet our style
>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.

>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.

>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!


>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.

>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.

>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.

-Ralph





More information about the Squeak-dev mailing list