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

Don't prefer Array.new(3) over 3.times.map #84

Merged
merged 1 commit into from
May 11, 2018
Merged

Conversation

lewispb
Copy link
Contributor

@lewispb lewispb commented May 10, 2018

@lewispb lewispb requested a review from balvig May 10, 2018 13:06
@karlentwistle karlentwistle self-requested a review May 10, 2018 13:56
Copy link
Contributor

@karlentwistle karlentwistle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

@errm
Copy link
Member

errm commented May 10, 2018

Without taking a view here the reason for this cop existing is because the Array.new form is more performant (apparently)

See: rubocop/rubocop#2583

I personally think that unless you are specifically optimising some code for performance you should always use the more readable form anyway ....

@lewispb lewispb requested a review from sogamoso May 10, 2018 14:26
Copy link
Contributor

@balvig balvig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks a lot! 👍

Knack
Knack previously requested changes May 11, 2018
Copy link
Member

@Knack Knack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the readability of 3.times.map { |x| } over Array.new(3).map { |x| }.

Well, it's really Array.new(3) { |x| }

I personally don't see any difference in readability. I would even say that I prefer the second: "Create an new array of size 3 and initialize it using the block"

instead of "map "3.times" with a block"

@balvig
Copy link
Contributor

balvig commented May 11, 2018

@Knack now sure if this convinces you but I found the original diff was easier to follow that way.

"Give me this thing 3 times"

@lewispb
Copy link
Contributor Author

lewispb commented May 11, 2018

@Knack sorry my mistake on the syntax.

@Knack
Copy link
Member

Knack commented May 11, 2018

I found the original diff was easier to follow that way.

But that's because the original code had a bug

Array.new(3).map {} means "Create an empty array and use it to map another array"

Whereas

Array.new(3) {} means "Create an array of size 3 and initialize it". Much simpler, IMHO.

Even if all the values are the same you can pass it as a second argument: Array.new(3, "x")

But as I said, I don't see a big difference and I don't care too much about performance. So it's ok to disable it.

@Knack Knack dismissed their stale review May 11, 2018 09:43

Either one seems good

@lewispb lewispb merged commit 3952e8e into master May 11, 2018
@lewispb lewispb deleted the disable-timesmap-cop branch May 11, 2018 09:43
@l15n
Copy link
Contributor

l15n commented May 14, 2018

Agree about the lack of need to optimize for performance here. I think I'd also fall into the habit of writing 3.times.map { }, but I'm convinced by Paco that Array.new(3) { } conveys the intent better.

@balvig
Copy link
Contributor

balvig commented May 14, 2018

Thanks for the extra explanation @Knack!

I also agree with this quote:

Either one seems good

In the end I think the PR title is maybe incorrect, but the fix is 👍 😉

@Knack
Copy link
Member

Knack commented May 14, 2018

Yes, it would be more appropriate "Don't prefer Array.new(3) over 3.times.map" 😄

@lewispb lewispb changed the title Prefer 3.times.map over Array.new(3).map Don't prefer Array.new(3) over 3.times.map May 14, 2018
@lewispb
Copy link
Contributor Author

lewispb commented May 14, 2018

Thanks for the suggestion @Knack I have changed it.

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.

7 participants