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.