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

Replaced all 'for' loops with Enumerable#each #4744

Merged

Conversation

ndbroadbent
Copy link
Contributor

Was reading a small discussion on r/ruby about for loops vs. Enumerable#each.

foo.each do |bar| is definitely the most common and preferred style, so I grepped the Rails codebase and replaced the 27 for loops that I found.

@spastorino
Copy link
Contributor

Seems fine to me but please don't change the code under vendor

@ndbroadbent
Copy link
Contributor Author

Fixed, sorry I missed that

spastorino added a commit that referenced this pull request Jan 29, 2012
…erables

Replaced all 'for' loops with Enumerable#each
@spastorino spastorino merged commit dfa0c36 into rails:master Jan 29, 2012
@jeremy
Copy link
Member

jeremy commented Jan 29, 2012

In most of these cases, for is easier to read than the #each equivalent.

No sense changing them wholesale because a Reddit thread says they're uncommon!

@spastorino
Copy link
Contributor

I've applied the patch not because of Reddit thread, probably I should've commented my thoughts first :(.
I've applied it because I find the code better to read but if you don't like feel free to revert it

@ndbroadbent
Copy link
Contributor Author

Thanks for the feedback. Now that I look at it again, I agree that for loops are easier to read when dealing with ranges.

Here's a pull request to revert to using for loops for ranges.

josevalim pushed a commit that referenced this pull request Jan 31, 2012
Fix GH #4744. Don't run bundle install when executing `rails new app` with --pretend option
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

Successfully merging this pull request may close these issues.

None yet

3 participants