On 18 juin 2013, at 11:20, Goubier Thierry wrote:

Hi Camille,

It's not bad as things goes, because it does look a bit like non-smalltalk code (i.e. the if then else structure) except that the precise meaning is very much Smalltalk :

Ok the name may be confusing.
So if you rename it to something like #if:do:elseDo:, it would be more smalltalkish and there is no confusion with classical if-then-else. 

if: has a receiver, and there is a not so intuitive relationship between the bloc parameters and the receiver.

Talking to an arbitrary object is the goal, since booleans are not very talkative kind of object.
Compare 

| temp |
temp := <complexExpression>
temp isNil ifTrue: [...] ifFalse: [...  temp ...]

with:

<complexExpression> ifNil: [ ... ] ifNotNil: [ :obj | ... obj ...]

ifNil:ifNotNil: is not better only because it is shorter, it is better because I am talking to the object I am auditing, not a stupid boolean that knows nothing else but its truth value.

Honestly, as far as code reviews goes, I'll prefer the temp refactoring. Understandable without having to search for the implementors of #if:then:else:

I know this would break a long time tradition and people may not like it, so that's a question of taste and style, not understandability. 
There would be only one implementor, the implementation is really simple and the selector is explicit. 
Anyone can get use to it in less than 2 min.
Do everyone verify the implementors of do: , collect: , ifTrue:ifFalse: ,... every time? 
No because people learned what they do when they learned Smalltalk, and I'm sure it didn't get them more than a few minutes.


Regards,

Thierry

Le 18/06/2013 10:51, Camille Teruel a �crit :
Hello everyone.

I see a lot of code that follows the following pattern:

<a long expression> <aComplexTest>
    ifTrue: [ <a long expression> <something> ]
    ifFalse: [ <a long expression> <something else> ].

The classic refactoring is to store <a long expression> in a temp to
avoid repetition:

| temp |
temp := <a long expression>.
temp <aComplexTest>
    ifTrue: [ temp <something> ]
    ifFalse: [ temp <something else> ].

Then <a long expression> is evaluated once and the name of the temp can
carry a meaningful name that helps reading the code.
But this is more verbose than the first version.
But if you add the following method in Object:

if: conditionBlock then: trueBlock else: falseBlock
    ^ (conditionBlock cull: self)
        ifTrue: [ trueBlock cull: value ]
        ifFalse: [ falseBlock cull: value ]

You can then rewrite it like that:

<a long expression>
    if: [ :obj | obj <aComplexTest> ]
    then: [ :obj | <something> ]
    else: [ :obj | <something else> ]

The names of the block args can give a meaningful name depending on the
result of the test (something like ... if: #notEmpty then: [ :collection
| ... ] else: [ :emptyCollection | ... ]).  Using cull: for the three
block also enable a good flexibility: you can omit the arg when irrelevant.

Likewise, we could also have Object>>#while:do:

while: conditionBlock do: actionBlock
    ^ [ conditionBlock cull: self ] whileTrue: [ actionBlock cull: self ]

What do you think?

Camille

--
Thierry Goubier
CEA list
Laboratoire des Fondations des Syst�mes Temps R�el Embarqu�s
91191 Gif sur Yvette Cedex
France
Phone/Fax: +33 (0) 1 69 08 32 92 / 83 95