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

Allow option for preferring for loop over each block #654

Closed
agrimm opened this issue Dec 3, 2013 · 5 comments
Closed

Allow option for preferring for loop over each block #654

agrimm opened this issue Dec 3, 2013 · 5 comments
Assignees

Comments

@agrimm
Copy link
Contributor

agrimm commented Dec 3, 2013

Currently, the For cop detects any use of for, and suggests replacing it with each. I'd like a configuration option that makes the cop instead suggest replacing a multi-line each block with a for loop.

Under that configuration, the following would be ok:

def launch_the_missiles
  @missiles.each(&:launch)
end

but the following would not be ok:

def launch_the_missiles
  @missiles.each do |missile|
    missile.arm
    missile.fire
  end
end

which should be transformed into

def launch_the_missiles
  for missile in @missiles
    missile.arm
    missile.fire
  end
end

As to why some people would prefer for loops over each?

Dave Thomas advocates for loops because they're more readable. https://twitter.com/JonRowe/status/306599505967607808

I prefer for loops because when you're using each, you're doing something for side-effects, rather than to get a value. I may as well make such code look different to functional programming (things like map and find_all), because I want potentially problematic code to look different from normal code, and I regard side-effects as potentially problematic.

This is probably not suitable for auto-correction, because scoping for for loops is different to that of each blocks.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 3, 2013

The scoping of for is the main reason I profoundly dislike it. I'll think for a while how best to approach this.

@lee-dohm
Copy link

lee-dohm commented Dec 3, 2013

@agrimm I have to assume that "more readable" in this case means "more familiar to users of other languages coming to Ruby for the first time".

On the other hand, your side-effects argument does hold some merit (though there are a couple other methods specifically for side-effects in Enumerable). I think it is counter-balanced by the fact that every other "iterating over a collection to do something" method follows the block form. And I think the admonishment against using for has less to do with the construct in-and-of-itself and more to do with breaking habits that prevent one from moving to the other Enumerable functions. I know when I first moved to Ruby, my over-reliance on for hindered me and only by forcing myself to use #each and then seeing how the code could easily be rewritten to #map, #reject, etc because of the similar structure was I able to make significant progress in creating new habits.

@jonas054
Copy link
Collaborator

jonas054 commented Dec 3, 2013

👍 on @agrimm's suggestion.

@lee-dohm The readability of for is not just about its similarity to the same construct in other languages. It's also that it's using two keywords instead of one (more color in your editor) and no punctuation (. | |). And it reads more like English. You just have to add the word 'each' in your head to get "for each missile in missiles", which makes sense. "Missiles each do missile" does not.

@bbatsov If the surrounding scope is small (we're enforcing 10 line methods by default), then the scoping is no big deal.

I still prefer each over for, and no-one has suggested we make for the default style. I just think it's reasonable for other people to prefer for.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 4, 2013

@jonas054 I don't mind adding this, if you're willing to implement it :-)

@jonas054
Copy link
Collaborator

jonas054 commented Dec 4, 2013

Yes I think I am. 😄

@ghost ghost assigned jonas054 Dec 4, 2013
bbatsov added a commit that referenced this issue Dec 7, 2013
[Fix #654] Add config option EnforcedStyle to For cop.
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