Special Formatting

Sometimes in a method you wind up wanting special formatting. We take it as a sign, instead, that you should rewrite the method. Here is a correctly formatted method according to KentBeck's SmalltalkBestPracticePatterns RectangularBlock? and IndentedControlFlow?:

    removeStep
        | stepToRemove |
        stepToRemove := self list selection.
        stepToRemove isNil ifFalse:
            [stepToRemove isExecutable ifTrue:
                [self list remove: stepToRemove.
                steps remove: stepToRemove]]

Let's face it, it’s ugly. A better solution is to refactor using SmalltalkBestPracticePatterns GuardClause, TypeSugestingParameterName?, QueryMethod and IntentionRevealingMethod?:

    removeStep
        self isNoStepSelected ifTrue: [^self].
        self removeStep: self selectedStep

removeStep: aStep aStep isNotExecutable ifTrue: [^self]. self list remove: aStep. steps remove: aStep

isNoStepSelected ^self selectedStep isNil

selectedStep ^self list selection

The result is better expression of intention and far simpler. The second method might even come in handy again, although that wasn't the point of the change.

Further, we now see that removeStep should really be named removeSelectedStep (as per SmalltalkBestPracticePatterns IntentionRevealingSelector). A good XPer, noting this, would grab all the senders and rename the method.


It seems like the real message here is to just use Kent's book.


Here is another example taken from a real system that was in production. Note that I have distilled the method down since it was originally about 50 lines long, if you can believe it. What remains here is the control structure only.

 startLoading
     | loadProcess loadStatusScreen |
     loadStatusScreen := StatusScreen? new.
     loadProcess := [[LoadProcess? new] valueNowOrOnUnwindDo: [loadStatusScreen exit]] forkAt: Processor userBackgroundPriority.
     loadStatusScreen process: loadProcess

You might be tempted to format it:

 startLoading
     | loadProcess loadStatusScreen |
     loadStatusScreen := StatusScreen? new.
     loadProcess := 
         [
             [
                 LoadProcess? new
             ] valueNowOrOnUnwindDo: 
                 [
                     loadStatusScreen exit
                 ]
          ] forkAt: Processor userBackgroundPriority.
     loadStatusScreen process: loadProcess

Or some other way to make where the various blocks begin and end obvious, but a better way is to break it down into smaller pieces using ComposedMethod and IntentionRevealingMethod? (from Kent's book):

 startLoading
     self startLoadingShowStatus: StatusScreen? new

startLoadingShowStatus: aStatusScreen aStatusScreen process: (self startLoadProcess: aStatusScreen)

startLoadProcess: aStatusScreen ^(self loadBlock: aStatusScreen) forkAt: self loadPriority

loadPriority ^Processor userBackGroundPriority

loadBlock: aStatusScreen ^[[LoadProcess? new] valueNowOrOnUnwindDo: [aStatusScreen exit]]

We no longer need formating to show us how this method actually works. Our methods each concentrate on doing only one thing.


In badly-written C++ the same thing often happens. For example, the worst of the windows apis and method calls have a whole host of parameters, and those who are forced to work with them write code that is specially formatted along the lines of:

     HRESULT hr = SomeFunctionName?(
               0,                  // Reserved, Parameter must be 0
               0,                  // Don't do some odd thing
               THIS|THAT|ANOTHER,  // Set some special flags
               BSTR(0),            // Reserved, must be as given
               &something);        // Object to be filled in with a value
This idiom is very common in all MS code, and frankly the version I give is very small. There are often ten or more parameters... another bad smell.


EditText of this page (last edited August 26, 1999) or FindPage with title or text search