On Tue, Jul 15, 2014 at 11:01 AM, rahul <[email protected]> wrote:
> [..snip..]
>
>
>> and then there's been some discussion on the PR around these two cops:
>>>
>>> Style/AndOr
>>> Lint/AssignmentInCondition
>>>
>>> Each of those two checks catch coding patterns which both are a source
>>> of some bugs and, at the same time are idiomatic in certain cases. So
>>> there's room for discussion on those two.
>>>
>>>
>> The only idiomatic part of the and/or one that we'd get rid of is failure
>> statements: do_something() or raise "Noooo!!!". I'm willing to drop that
>> for the slightly more wordy:
>>
>> if !do_something()
>> raise "Nooooo!!!!"
>> end
>>
>> In fact, I often like that construction better because it is more future
>> proof. If it turns out that you need to add extra actions in the error
>> case, then there isn't any need to perform refactor first.
>>
>
> It is possible to get back most idiomatic constructions by slightly
> tweaking the cops. In fact it is rather easy to do so, and here[1] is a
> custom cop that checks for And/Or only in conditionals.
> - I feel that the boolean control flow idiom is actually useful.
>
> A pattern that I have seen often in code is of the type
>
> unless x = mymethod()
> x.do_something
> end
>
>
I think you unwittingly just provided an example of exactly why I like
neither assignments in expressions nor unless. What you just wrote is,
"call do_something on x if x is nil or false". It is really easy to write
that unintentionally with assignments and unless combined. :)
> Which falls foul of Lint/AssignmentInCondition
>
> I think that it would be better written as
> `x = mymethod() and x.do_something`
> rather than
> `(x = mymethod()) && x.do_something`
> which is recommended in the ruby style guide from rubocop if we restrict
> and/or to purely control flow operators.
>
>
This is starting to get into territory of religious wars, but I think we
should be looking for what construction is the most obviously correct from
the standpoint of a reader. I find that "and" isn't the most
straightforward nor easily changed construction. For instance, your example
could be written without "and" as:
x = mymethod()
if x
x.do_something()
end
Now, granted, that is 4 lines instead of 1, but it also makes a few things
really obvious to me:
* There isn't an else clause. Did the author forget that? Maybe it
doesn't really matter? Is there some construction later on that is
duplicating the equivalent of the else clause that should be refactored to
be combined with this?
* There is clearly a variable x that is being created and bound to a value
* x is used later on
* A decision is made based on the value of x. This one brings up the
question of if the decision needed in the first place.
* When there is another piece of information that needs to affect the
call to do_something, there is a clear place to put it.
> [1]
> https://github.com/vrthra/puppet/commit/07cced3157a957b1b49de00a34da075af580fad7
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Puppet Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [email protected].
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/puppet-dev/d3442dc7-2193-44aa-a000-18ecc2327082%40googlegroups.com
> <https://groups.google.com/d/msgid/puppet-dev/d3442dc7-2193-44aa-a000-18ecc2327082%40googlegroups.com?utm_medium=email&utm_source=footer>
> .
>
> For more options, visit https://groups.google.com/d/optout.
>
--
Andrew Parker
[email protected]
Freenode: zaphod42
Twitter: @aparker42
Software Developer
*Join us at PuppetConf 2014 <http://www.puppetconf.com/>, September
22-24 in San Francisco*
*Register by May 30th to take advantage of the Early Adopter discount
<http://links.puppetlabs.com/puppetconf-early-adopter> **—**save $349!*
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/puppet-dev/CANhgQXveN_tTRTeDv0_KHLuyFVnTZ9bMHA%2B0-K1Z0a7VAL32AA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
