Tuesday, September 20, 2005

Re: Dolphin's Command Framework

I got a comment from Anand that suggested that I change this code:
CommandMessageSend>>queryCommand: query 
| canSelector |
canSelector := ('can' , query command selector capitalized) asSymbol.
^(self receiver respondsTo: canSelector)
ifTrue:
[query
isEnabled: (self receiver perform: canSelector);
receiver: self receiver.
true]
ifFalse: [super queryCommand: query]

to:
CommandMessageSend>>queryCommand: query
| canSelector isEnabled |
canSelector := ('can' , query command selector capitalized) asSymbol.
[isEnabled := self receiver perform: canSelector]
on: MessageNotUnderstood
do: [:ex | ^super queryCommand: query].
query
isEnabled: isEnabled;
receiver: self receiver.
^true

We got rid of the ifTrue:ifFalse: and the code is clearer. But, something bothered me about it. The MessageNotUnderstood could catch any MessageNotUnderstood further down in the stack (it might not be my canCommandSelector). What is a Smalltalker to do? How about this:
Object>>perform: aSelector onNotUnderstoodDo: aBlock
^[self perform: aSelector]
on: MessageNotUnderstood
do: [:ex | (ex receiver == self and: [ex selector == aSelector])
ifTrue: [aBlock value]
ifFalse: [ex pass]]

This message allows us to do the perform, but check to make sure it's the one we care about. Now, our CommandMessageSend code looks like this:
CommandMessageSend>>queryCommand: query
| canSelector isEnabled |
canSelector := ('can' , query command selector capitalized) asSymbol.
isEnabled := self receiver perform: canSelector onNotUnderstoodDo: [^super queryCommand: query].
query
isEnabled: isEnabled;
receiver: self receiver.
^true

The code is about the same, but now we are only catching the MessageNotUnderstood exception that we care about and not any others. The code is still clean and we see another example of the power of Smalltalk exceptions. And I would like to thank Anand for the suggestion!

4 comments:

Anonymous said...

this looks good and safe.
small nitpick (my apologies for being so)
in onNotUnderstoodDo: the ifTrue: part can be just ifTrue: aBlock rather than ifTrue: [aBlock value]
same result; one less block evaluation. or am I missing something ?
Regards
Anand

Blaine Buxton said...

I actually I thought about it, but decided not to because I've noticed it freaks out some programmers because they're used to seeing that extra block. So, to make it more readable, I optted for the ifTrue: [aBlock value] instead of ifTrue: aBlock instead. You are right though. It is one more block evaluation that is unneeeded. I often err on the side of readability, but it is subjective. Your mileage may vary.

Anonymous said...

this got me thinking of cases where
[[aBlock value]]value would be different from [aBlock] value.
if i had subclassed Block and if my class had overridden value to say return another block; the above would cause an issue ?

pretty contrived example i know
i shud go do a quick check

Anonymous said...

Blaine,

I tried to get this to work with Menus and ContextMenus in the ViewComposer, without success.

Could you give us a for dummies description how to do that?

Thanks,

Günther, guenni