Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The Style/AndOr rule autofix prevents using "and" and "or" as control-flow modifiers #73

Closed
patbl opened this issue Feb 4, 2019 · 6 comments

Comments

@patbl
Copy link

patbl commented Feb 4, 2019

I have some code like this:

anchor_elements.map { |element|
  href = element[:href] or next
  url = URI.join("https://www.youtube.com", href).to_s
}.compact

Standard changes it to this:

anchor_elements.map { |element|
  (href = element[:href]) || next
  url = URI.join("https://www.youtube.com", href).to_s
  Video[url: url, title: element.content]
}.compact

Does anyone write code like that? I think using || and && for control flow is a bad idea because you need to pay close attention to how things are parenthesized. Maybe the idea is that you'd get used to your code being autocorrected this way and choose to write it a different way in the future.

Here are a couple of other ways to write it:

anchor_elements.map { |element|
  href = element[:href]
  next unless href
  url = URI.join("https://www.youtube.com", href).to_s
  Video[url: url, title: element.content]
}.compact

I guess that's OK; it just chops up one line into two without a good reason.

This requires two blocks:

anchor_elements.select { |element| element[:href] }.map { |element|
  url = URI.join("https://www.youtube.com", element[:href]).to_s
  Video[url: url, title: element.content]
}

I frequently use and and or for control flow, and I'd miss them if I weren't able to use them. Maybe their misuse in boolean expressions is bad enough to outweigh their succinctness and expressiveness in control flow.

I appreciate what you're doing with this project!

@danielmorrison
Copy link
Contributor

I had a tough time understanding what's happening in the first example, which leads me to think keeping this cop is a good idea.

I've become less of a fan of and and or over time. The fact that the logic changes at all here by using or vs || is exactly why. It can lead to unintended bugs and confusion.

My suggestion is to keep standard as-is, and suggest you turn off that cop in your config. Just my 2¢.

@searls
Copy link
Contributor

searls commented Feb 4, 2019

Yes, I agree with @danielmorrison. I appreciate the thought put into the OP, but I don't see very many defenders of and and or in production ruby code these days because of the precedence confusion they can cause

@searls searls closed this as completed Feb 4, 2019
@mvz
Copy link

mvz commented Mar 6, 2019

FWIW, I really appreciate the use of and and or in control flow.

The different precedence is a feature. If there was no difference, the existence of these different operators wouldn't make sense. Compare the difference between the different block delimiters.

The different precedence is what allows for code like

foo = bar(baz, qux) or raise 'No bar found'

Blanket disallowing or means you cannot use logic operators for control flow in practice, which I see as a loss. The alternatives are worse to me:

foo = bar(baz, qux)
raise 'No bar found' unless foo # Too much emphasis on an error condition
foo = bar(baz, qux)
 # Way too many lines for an error condition
unless foo
  raise 'No bar found'
end

@mvz
Copy link

mvz commented Oct 31, 2019

I had a tough time understanding what's happening in the first example

@danielmorrison do you find any of the 'corrections' easier to understand? Or how would you write it otherwise?

@danielmorrison
Copy link
Contributor

I prefer either of your alternatives to the original or raise.

I've been bit (recently even) by code where changing and to && altered the logic unexpectedly.

I get that or raise is pretty, but it isn't worth the confusion to me.

@searls
Copy link
Contributor

searls commented Nov 4, 2019

I still agree with Daniel. The precedence changes in and and or take them beyond mere intention-revealing syntactic sugar and into oft-deployed footguns. Because people (1) know there's a difference, (2) don't know what the difference is, I see lots of folks completely stall as soon as they see an and or or

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants