Critique my code

danielv at netvision.net.il danielv at netvision.net.il
Wed Oct 31 16:40:46 UTC 2001


Dennis, that's cool!
I already have a few uses for it, if you do extend it to accept more
kinds of sources - I want to do a POP mailbox checker, and a current
changeset size indicator, for example...

Sam already gave you lots of good things worth doing, so I'll just add
my two cents.

Basically the changes Sam indicates are refactorings - changes to the
code that mostly don't change functionality, but make the code easier to
understand and thus improve. Two problems with this sort change -
they're sometimes tedious, and they give you opportunities to add bugs.

For example, you have a class AbstractMeter with variables named
meterZones, meterLow and so forth. The context is already clear enough
from the class you're writing in - you can rename the variables,
removing the repetitive prefix. This would be an imporvement, but to do
each rename, you'd have to find and change manually each reference to
each variable - lots of work, and if you forget an occurance, that's a
bug.

Enter the Refactoring Browser.
This is a tool that looks and behaves like the regular system browsers,
except that it supports and automates many common refactorings. For
example, Rename Instance Variable. Also Extract Method, which is the
easiest way to make you methods smaller.

Sam Adams <ssadams at us.ibm.com> wrote:
> 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