Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule
Locked
 
|
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 |
Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule
|
|
Hi Yuriy, to me the first form is a list of simple rules.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 ] ] ] 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. |
Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule
|
|
2017-04-07 11:03 GMT+02:00 Nicolas Cellier <[hidden email]>:
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.
|
Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule
|
|
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 |
Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule
|
|
2017-04-07 11:23 GMT+02:00 [hidden email] <[hidden email]>:
Me too |
|
|
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 |
|
|
of course better way to deal with this is to use double dispatch, but you cannot always do it ;)
|
|
|
In reply to this post by Marcus Denker-4
+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
|
|
|
The Boolean expression is easier to extract methods with an intention revealing name. The "case with returns " it's harder to automatically refactor. On Apr 7, 2017 07:06, "Yuriy Tymchuk" <[hidden email]> wrote:
|
Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule
|
|
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 |
Re: Rationale behind the "ifTrue:/ifFalse: returns instead of and:/or:'s" rule
|
|
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 |
«
Return to Pharo Smalltalk Developers
|
1 view|%1 views
| Free forum by Nabble | Edit this page |