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

Style cop idea: prefer map over each + << #11878

Closed
vlad-pisanov opened this issue May 14, 2023 · 2 comments · Fixed by #12584
Closed

Style cop idea: prefer map over each + << #11878

vlad-pisanov opened this issue May 14, 2023 · 2 comments · Fixed by #12584

Comments

@vlad-pisanov
Copy link

I often encounter this pattern, especially amongst junior devs; they start with the [] literal, and then build up a list by repeatedly pushing elements into it. It's messy and inefficient.

# bad
result = []
arr.each { |x| result << x + 42 }

# good
result = arr.map { |x| x + 42 }
@natematykiewicz
Copy link

natematykiewicz commented Jun 5, 2023

Is there a way to detect that multiple loops aren't pushing to a single array? Because I sometimes do this to cut down on array allocations. This cop would force me to use map instead of each, and then I'd have to concatenate the arrays together still, which results in N more array allocations and also N more object iterations (+ has to iterate the result list again during concatenation).

result = []
arr1.each { |x| result << x if something }
arr2.each { |x| result << x if something }

ymap added a commit to ymap/rubocop that referenced this issue Dec 30, 2023
Fixes rubocop#11878.

This PR adds new `Style/MapIntoArray` cop.
This cop checks for usages of `each` with `<<`, `push`, or `append`
which can be replaced by `map`. It is unsafe, because it can produce
false positives if the receiver is not an `Enumerable`.

```ruby
dest = []
src.each { |e| dest << e * 2 }
dest

src.map { |e| e * 2 }

dest = []
src.each { |e| dest << e * 2; puts e  }
dest
```
ymap added a commit to ymap/rubocop that referenced this issue Dec 30, 2023
Fixes rubocop#11878.

This PR adds new `Style/MapIntoArray` cop.
This cop checks for usages of `each` with `<<`, `push`, or `append`
which can be replaced by `map`. It is unsafe, because it can produce
false positives if the receiver is not an `Enumerable`.

```ruby
# bad
dest = []
src.each { |e| dest << e * 2 }
des

# good
src.map { |e| e * 2 }

# good - contains another operation
dest = []
src.each { |e| dest << e * 2; puts e  }
dest
```
@ymap
Copy link
Contributor

ymap commented Dec 30, 2023

I've had some hesitations, but I decided to go ahead and create the PR. The following points were particularly complex. I would really appreciate any feedback.

  • Given that Enumerable#map returns an Array, I limited the cop's reporting to cases with a statically determinable Array as the push destination.

  • As previously noted, due to efficiency concerns, I restricted the cop's reporting to cases where the Array being pushed to was clearly initialized as empty. Of course, using += for non-empty Array was considered, but I feel it's more appropriate as a specific option in the cop, if needed.

  • For cases where each's return value might be used, I skipped autocorrection. Although rare, prioritizing safety over simplicity seemed crucial.

ymap added a commit to ymap/rubocop that referenced this issue Dec 31, 2023
Fixes rubocop#11878.

This PR adds new `Style/MapIntoArray` cop.
This cop checks for usages of `each` with `<<`, `push`, or `append`
which can be replaced by `map`. It is unsafe, because it can produce
false positives if the receiver is not an `Enumerable`.

```ruby
# bad
dest = []
src.each { |e| dest << e * 2 }
dest

# good
src.map { |e| e * 2 }

# good - contains another operation
dest = []
src.each { |e| dest << e * 2; puts e  }
dest
```
ymap added a commit to ymap/rubocop that referenced this issue Jan 23, 2024
Fixes rubocop#11878.

This PR adds new `Style/MapIntoArray` cop.
This cop checks for usages of `each` with `<<`, `push`, or `append`
which can be replaced by `map`. It is unsafe, because it can produce
false positives if the receiver is not an `Enumerable`.

```ruby
# bad
dest = []
src.each { |e| dest << e * 2 }
dest

# good
src.map { |e| e * 2 }

# good - contains another operation
dest = []
src.each { |e| dest << e * 2; puts e  }
dest
```
ymap added a commit to ymap/rubocop that referenced this issue Jan 24, 2024
Fixes rubocop#11878.

This PR adds new `Style/MapIntoArray` cop.
This cop checks for usages of `each` with `<<`, `push`, or `append`
which can be replaced by `map`. It is unsafe, because it can produce
false positives if the receiver is not an `Enumerable`.

```ruby
# bad
dest = []
src.each { |e| dest << e * 2 }
dest

# good
dest = src.map { |e| e * 2 }

# good - contains another operation
dest = []
src.each { |e| dest << e * 2; puts e  }
dest
```
ymap added a commit to ymap/rubocop that referenced this issue Jan 24, 2024
Fixes rubocop#11878.

This PR adds new `Style/MapIntoArray` cop.
This cop checks for usages of `each` with `<<`, `push`, or `append`
which can be replaced by `map`. It is unsafe, because it can produce
false positives if the receiver is not an `Enumerable`.

```ruby
# bad
dest = []
src.each { |e| dest << e * 2 }
dest

# good
dest = src.map { |e| e * 2 }

# good - contains another operation
dest = []
src.each { |e| dest << e * 2; puts e }
dest
```
bbatsov pushed a commit that referenced this issue Apr 8, 2024
Fixes #11878.

This PR adds new `Style/MapIntoArray` cop.
This cop checks for usages of `each` with `<<`, `push`, or `append`
which can be replaced by `map`. It is unsafe, because it can produce
false positives if the receiver is not an `Enumerable`.

```ruby
# bad
dest = []
src.each { |e| dest << e * 2 }
dest

# good
dest = src.map { |e| e * 2 }

# good - contains another operation
dest = []
src.each { |e| dest << e * 2; puts e }
dest
```
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 a pull request may close this issue.

3 participants