Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

Uko2
Hi, there is a rule that suggests to use and/or boolean operations instead of multiple returns.

For example it suggests agains using:

        isTranslucentButNotTransparent
               
                backgroundColor ifNil: [ ^ true ].
                (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
                (borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
                ^ false

Instead you should use:

        isTranslucentButNotTransparent
               
                ^ backgroundColor isNil or: [
                (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
                borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]

And at least a few developers think that the suggested implementation is more complicated to comprehend.

What is the reasoning behind the rule? Does the suggested version run faster (i.e. is optimized ing some way?).

Cheers.
Uko
Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

Nicolas Cellier
Hi Yuriy,
to me the first form is a list of simple rules.
The second one is a complex expression

You made an effort to keep the list format for the second expression, while violating traditional block formatting which would be



     ^ backgroundColor isNil
or: [
                (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
                borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]

I don't think that runtime optimization count at all in such choice.

2017-04-07 10:33 GMT+02:00 Yuriy Tymchuk <[hidden email]>:
Hi, there is a rule that suggests to use and/or boolean operations instead of multiple returns.

For example it suggests agains using:

        isTranslucentButNotTransparent

                backgroundColor ifNil: [ ^ true ].
                (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
                (borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
                ^ false

Instead you should use:

        isTranslucentButNotTransparent

                ^ backgroundColor isNil or: [
                (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
                borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]

And at least a few developers think that the suggested implementation is more complicated to comprehend.

What is the reasoning behind the rule? Does the suggested version run faster (i.e. is optimized ing some way?).

Cheers.
Uko

Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

Nicolas Cellier


2017-04-07 11:03 GMT+02:00 Nicolas Cellier <[hidden email]>:
Hi Yuriy,
to me the first form is a list of simple rules.
The second one is a complex expression

You made an effort to keep the list format for the second expression, while violating traditional block formatting which would be



     ^ backgroundColor isNil
or: [
                (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
                borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]


Sorry, premature post caused by missuse of tab key, traditional format could be something like:

    isTranslucentButNotTransparent

        ^ backgroundColor isNil
                   or:
                       [ (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ])
                           or:
                               [ borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]

Personnally, I like this indentation better.
It's still preserving the list of simple rules, and indicate a hierarchy among the rules
But indentations sounds overkill and won't scale very high.

To me the first form is nearer the pattern matching style of a functional language.
 
I don't think that runtime optimization count at all in such choice.

2017-04-07 10:33 GMT+02:00 Yuriy Tymchuk <[hidden email]>:
Hi, there is a rule that suggests to use and/or boolean operations instead of multiple returns.

For example it suggests agains using:

        isTranslucentButNotTransparent

                backgroundColor ifNil: [ ^ true ].
                (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
                (borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
                ^ false

Instead you should use:

        isTranslucentButNotTransparent

                ^ backgroundColor isNil or: [
                (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
                borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]

And at least a few developers think that the suggested implementation is more complicated to comprehend.

What is the reasoning behind the rule? Does the suggested version run faster (i.e. is optimized ing some way?).

Cheers.
Uko


Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

Marcus Denker-4
In reply to this post by Uko2

> On 7 Apr 2017, at 10:33, Yuriy Tymchuk <[hidden email]> wrote:
>
> Hi, there is a rule that suggests to use and/or boolean operations instead of multiple returns.
>
> For example it suggests agains using:
>
> isTranslucentButNotTransparent
>
> backgroundColor ifNil: [ ^ true ].
> (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
> (borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
> ^ false
>
> Instead you should use:
>
> isTranslucentButNotTransparent
>
> ^ backgroundColor isNil or: [
> (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
> borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]
>
> And at least a few developers think that the suggested implementation is more complicated to comprehend.
>

I really do not like the complex boolean expressions… my brain can understand the “check and return” guards extremely fast:

                backgroundColor ifNil: [ ^ true ].
                (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
                (borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
                ^ false

while for the nesting everything in one huge or: /and: expression I have to think. I always feel that replacing the “guard”
style with complex nested booleans makes the code worse.

I really would like to remove the rule.

        Marcus
Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

Denis Kudriashov

2017-04-07 11:23 GMT+02:00 [hidden email] <[hidden email]>:
I really do not like the complex boolean expressions… my brain can understand the “check and return” guards extremely fast:

                backgroundColor ifNil: [ ^ true ].
                (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
                (borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
                ^ false

while for the nesting everything in one huge or: /and: expression I have to think. I always feel that replacing the “guard”
style with complex nested booleans makes the code worse.

I really would like to remove the rule.

Me too
Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

EstebanLM
In reply to this post by Uko2

> On 7 Apr 2017, at 10:33, Yuriy Tymchuk <[hidden email]> wrote:
>
> Hi, there is a rule that suggests to use and/or boolean operations instead of multiple returns.
>
> For example it suggests agains using:
>
> isTranslucentButNotTransparent
>
> backgroundColor ifNil: [ ^ true ].
> (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
> (borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
> ^ false
>
> Instead you should use:
>
> isTranslucentButNotTransparent
>
> ^ backgroundColor isNil or: [
> (backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
> borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]
>
> And at least a few developers think that the suggested implementation is more complicated to comprehend.
>
> What is the reasoning behind the rule? Does the suggested version run faster (i.e. is optimized ing some way?).

I disagree with such rule. Is confusing and unnecessary.

The “escape condition” is clearer and easier to understand.
And methods that are composed like this:

a = x1 ifTrue: [ ^ r1 ].
a = x2 ifTrue: [ ^ r2 ].

etc.

are very common because they are (again) legible and can be easily optimised by the JIT.

(they are even called “case methods” somewhere I do not remember :P)

Esteban

>
> Cheers.
> Uko


Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

EstebanLM

On 7 Apr 2017, at 11:59, Esteban Lorenzano <[hidden email]> wrote:


On 7 Apr 2017, at 10:33, Yuriy Tymchuk <[hidden email]> wrote:

Hi, there is a rule that suggests to use and/or boolean operations instead of multiple returns.

For example it suggests agains using:

isTranslucentButNotTransparent

backgroundColor ifNil: [ ^ true ].
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
(borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
^ false

Instead you should use:

isTranslucentButNotTransparent

^ backgroundColor isNil or: [
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]

And at least a few developers think that the suggested implementation is more complicated to comprehend.

What is the reasoning behind the rule? Does the suggested version run faster (i.e. is optimized ing some way?).

I disagree with such rule. Is confusing and unnecessary. 

The “escape condition” is clearer and easier to understand. 
And methods that are composed like this: 

a = x1 ifTrue: [ ^ r1 ].
a = x2 ifTrue: [ ^ r2 ].

etc. 

are very common because they are (again) legible and can be easily optimised by the JIT. 

(they are even called “case methods” somewhere I do not remember :P)

of course better way to deal with this is to use double dispatch, but you cannot always do it ;) 


Esteban


Cheers.
Uko

Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

EstebanLM
In reply to this post by Marcus Denker-4

On 7 Apr 2017, at 11:23, [hidden email] <[hidden email]> wrote:


On 7 Apr 2017, at 10:33, Yuriy Tymchuk <[hidden email]> wrote:

Hi, there is a rule that suggests to use and/or boolean operations instead of multiple returns.

For example it suggests agains using:

isTranslucentButNotTransparent

backgroundColor ifNil: [ ^ true ].
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
(borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
^ false

Instead you should use:

isTranslucentButNotTransparent

^ backgroundColor isNil or: [
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]

And at least a few developers think that the suggested implementation is more complicated to comprehend.


I really do not like the complex boolean expressions… my brain can understand the “check and return” guards extremely fast:

backgroundColor ifNil: [ ^ true ].
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
(borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
^ false

while for the nesting everything in one huge or: /and: expression I have to think. I always feel that replacing the “guard” 
style with complex nested booleans makes the code worse.

I really would like to remove the rule.

+42


Marcus

Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

Uko2

On 7 Apr 2017, at 12:01, Esteban Lorenzano <[hidden email]> wrote:


On 7 Apr 2017, at 11:23, [hidden email] <[hidden email]> wrote:


On 7 Apr 2017, at 10:33, Yuriy Tymchuk <[hidden email]> wrote:

Hi, there is a rule that suggests to use and/or boolean operations instead of multiple returns.

For example it suggests agains using:

isTranslucentButNotTransparent

backgroundColor ifNil: [ ^ true ].
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
(borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
^ false

Instead you should use:

isTranslucentButNotTransparent

^ backgroundColor isNil or: [
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]

And at least a few developers think that the suggested implementation is more complicated to comprehend.


I really do not like the complex boolean expressions… my brain can understand the “check and return” guards extremely fast:

backgroundColor ifNil: [ ^ true ].
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
(borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
^ false

while for the nesting everything in one huge or: /and: expression I have to think. I always feel that replacing the “guard” 
style with complex nested booleans makes the code worse.

I really would like to remove the rule.

+42

1) it’s not only about removing. Maybe we should detect complicated boolean expressions that return something and suggest to break them down.
2) I just want to know why someone created such rule :). Because the author of the last update of the initialize method is Marcus :)

Uko



Marcus


Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

gcotelli
The Boolean expression is easier to extract methods with an intention revealing name. The "case with returns " it's harder to automatically refactor. 

And if you get rid of the nils, it became easier to read and refactor. 😉

On Apr 7, 2017 07:06, "Yuriy Tymchuk" <[hidden email]> wrote:

On 7 Apr 2017, at 12:01, Esteban Lorenzano <[hidden email]> wrote:


On 7 Apr 2017, at 11:23, [hidden email] <[hidden email]> wrote:


On 7 Apr 2017, at 10:33, Yuriy Tymchuk <[hidden email]> wrote:

Hi, there is a rule that suggests to use and/or boolean operations instead of multiple returns.

For example it suggests agains using:

isTranslucentButNotTransparent

backgroundColor ifNil: [ ^ true ].
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
(borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
^ false

Instead you should use:

isTranslucentButNotTransparent

^ backgroundColor isNil or: [
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) or: [
borderColor isColor and: [ borderColor isTranslucentButNotTransparent ] ] ]

And at least a few developers think that the suggested implementation is more complicated to comprehend.


I really do not like the complex boolean expressions… my brain can understand the “check and return” guards extremely fast:

backgroundColor ifNil: [ ^ true ].
(backgroundColor isColor and: [ backgroundColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
(borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
^ false

while for the nesting everything in one huge or: /and: expression I have to think. I always feel that replacing the “guard” 
style with complex nested booleans makes the code worse.

I really would like to remove the rule.

+42

1) it’s not only about removing. Maybe we should detect complicated boolean expressions that return something and suggest to break them down.
2) I just want to know why someone created such rule :). Because the author of the last update of the initialize method is Marcus :)

Uko



Marcus



Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

Marcus Denker-4
In reply to this post by Uko2
>
> 1) it’s not only about removing. Maybe we should detect complicated boolean expressions that return something and suggest to break them down.

No idea…

> 2) I just want to know why someone created such rule :). Because the author of the last update of the initialize method is Marcus :)
>

I did not add it originally.

        Marcus


Reply | Threaded
Open this post in threaded view
|

Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule

John Brant-2
In reply to this post by Uko2

> 2) I just want to know why someone created such rule :). Because the author of the last update of the initialize method is Marcus :)

Probably to stop people from writing code like this:

        (borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]) ifTrue: [ ^ true ].
        ^ false

instead of:

        ^borderColor isColor and: [ borderColor isTranslucentButNotTransparent ]

:)


John Brant