Critique my code

Sam Adams ssadams at us.ibm.com
Tue Oct 30 16:35:25 UTC 2001


Dennis,
That's a significant accomplishment for your first coding effort in Squeak!
Many years ago I wrote and sold the first commercial gauges/meters
framework for Smalltalk (PluggableGauges- 1987), and I must say I am
impressed with all the functionality you have included (much more than I
did!).

A few suggestions:

**Method size
 In general Smalltalk/Squeak methods are only a few lines long.  Years ago
at Knowledge Systems we ran stats on methods across the Digitalk image and
found that the average number of messages sent in a method was only 6!
That's not lines of code, but individual messages.  While its OK to have
longer methods for reasons like performance(very, very rare) or algorithmic
clarity, your code will typically be much more readable and reusable if it
is broken up into smaller methods that have nice "intention revealing"
names, as Ward Cunningham used to say (and still does, I imagine!).  Try
rewriting your draw method to separate scale drawing from indicator/needle
drawing, and perhaps within your scale drawing method, split it between
major/minor tics and labels.  This will give users who want to subclass and
extend your code lots of easy places to override specific aspects of the
drawing without cloning the entire drawing code.

**RedZones and parsing
Keeping your red zone data in string form and constantly reparsing it in
various methods is unnecessary and overly complex, especially since Squeak
is about objects, not data structures.  Why not simply create a new class
called AlarmZone or something like it and let it model the zone range,
color and possibly an alarm action to be executed when the value of the
meter is in range.   In your properties dialog, you can keep the same kind
of textual specification for zones (with some simplification...see below)
and then on OK/Apply, parse it once and create and save the appropriate
zone objects in your meter to later inform the draw methods.

**RedZone specification made easier
Squeak has lots of ways to parse and manipulate strings, but for simple
data structure entry like (10,20)(90,100) you can let the compiler do the
work for you.  For instance, you could convert the string '(10 20)(90 100)'
into an array of arrays of numbers with the folowing code:

inputString := '(10 20)(90 100)'.
correctSyntaxString := '#(',inputString,')'.
array := Compiler evaluate: correctSyntaxString .

in other words, you let the user enter Smalltalk syntax or somethign close
and then simply evaluate the text into an object.  Hey, since you are using
the Smalltalk compiler anyway, you could even have the user enter something
like:

{AlarmZone min: 10 max: 20 color: Color blue action:[Broker buyStock].
AlarmZone min: 90 max: 100 color: Color red action:[Broker sellStock]}

And the compiler expression above will return an array of AlarmZone objects
ready to be saved in the meter.  End user programming on the cheap!

**Display performance
Last critique, since most of your submorphs don't change each step, don't
remove and recreate them each step.  Even with the needle, you only need to
update it if the value has changed since last time, unless of course the
user is moving or scaling the whole meter, in which case you need to redo
everything.

**Enhancement suggestion
Why limit your source to a URL?  You could again let the user enter a
Smalltalk expression that you assume results in a number when evaluated.
Then use the 'Compiler evaluate: theSource' approach each time you need the
value.  This turns your meters into a more general gauges package that can
be used for any value in Squeak.

To summarize:
In general, use more, smaller methods with intention revealing names.
Make first class objects for your data structures and give them appropriate
behavior.
Let Squeak do the parsing work whenever possible (and practical from a user
skill standpoint).
Don't redraw/rebuild morphs unless necessary.
and of course,
Keep on squeaking!

Regards,
Sam




Dennis Damico wrote:
<<
Hi all,


I'm new to Squeak and Smalltalk. Enclosed is my first coding effort. I
invite you experts to critique my code.


Have I chosen morphs and methods appropriately? Is the implementation
efficient? Does the style look like an acceptable Smalltalk style? Your
feedback and helpful suggestions will be appreciated.


The code was developed under Squeak 3.0 on Windows.


thanks


< code elided>


>>




Sam S. Adams, IBM Distinguished Engineer, IBM Research
tie line 444-0736, outside 919-254-0736, email: ssadams at us.ibm.com
<<Hebrews 11:6, Proverbs 3:5-6, Romans 1:16-17, I Corinthians 1:10>>





More information about the Squeak-dev mailing list