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

Remove the usage of &method #3826

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@rrosenblum
Copy link
Contributor

rrosenblum commented Dec 25, 2016

&method is a weird syntax in Ruby. It is also slower than an equivalently written function. I was hoping to update Style/SymbolProc to recommend against this syntax. Unfortunately, it may take a bit more work than I was hoping for. If you would like, I can create an issue for a feature request to track this and allow anyone to pick it up.

http://stackoverflow.com/questions/6976629/is-the-methodmethod-name-idiom-bad-for-perfomance-in-ruby

@backus

This comment has been minimized.

Copy link
Contributor

backus commented Dec 25, 2016

I was hoping to update Style/SymbolProc to recommend against this syntax

If you do, can you include configuration for disabling recommending against this syntax (or even maybe enforcing using it)? I personally like the syntax since, once you are used to the syntax, it is a nice visual cue that a local method is being invoked with exactly one argument.

@rrosenblum

This comment has been minimized.

Copy link
Contributor

rrosenblum commented Dec 25, 2016

can you include configuration for disabling recommending against this syntax

That sounds reasonable to me. I can see where the syntax could be nice. It is a bit more obvious what is going on than some of the other syntax that Ruby supports.

Out of curiosity, since you are a fan of the syntax, what are your thoughts on the following usage?

filenames = ["a.txt", "b.txt", "c.txt"]
texts = filenames.map(&File.method(:read))
@backus

This comment has been minimized.

Copy link
Contributor

backus commented Dec 25, 2016

Out of curiosity, since you are a fan of the syntax, what are your thoughts on the following usage?

filenames = ["a.txt", "b.txt", "c.txt"]
texts = filenames.map(&File.method(:read))

I like it. I think RuboCop would probably be enforcing it if we could write it as

filenames.map(&File->read)

or some nice looking special syntax. &method(:name) is ugly though especially to people who aren't familiar so I think it doesn't get much love (also yes it is slower but I personally don't write code that is ever close to bound by the execution speed of ruby itself).

I don't usually like getting too deep into explaining why I like certain syntax because I think it is easy to start unintentionally rationalizing, but with that disclaimer...

I'll focus on this snippet because you usually don't have the immediate context of the type of filenames in real world code like this. Looking at

filenames.map { |filename| ... }

or

filenames.map { |*args| ... }

I wonder about exactly what is being yielded.

Also, a lot of ruby methods take variable arguments. IO.read takes 1 through 4 arguments (more if you count each hash option). When we iterate and yield a block, I have to double check that we aren't using variable arguments somehow. I know this wont happen if I'm doing some_array.map(&method(:foo)) because I know this is equivalent to some_array.map { |item| foo(item) }. Basically, knowing that only one method is being invoked with one argument per item in the array is comforting to me.

@rrosenblum

This comment has been minimized.

Copy link
Contributor

rrosenblum commented Dec 25, 2016

Thanks for the response. The &method syntax is new to me as of a few weeks ago. I agree that the syntax with chained methods, and even a single method, could be improved with some short hand. At the same time, short hand starts to become more cryptic to someone that has never seen the syntax before.

also yes it is slower but I personally don't write code that is ever close to bound by the execution speed of ruby itself

I agree that something like the &method syntax probably isn't the bottle neck of an application. I have liked that we have added performance cops to RuboCop because it allows us to write code however we want and then let something automatically optimize some of the code for us.

Basically, knowing that only one method is being invoked with one argument per item in the array is comforting to me.

I can see where this could be beneficial in some situations.

@Drenmi

This comment has been minimized.

Copy link
Collaborator

Drenmi commented Dec 27, 2016

I think RuboCop would probably be enforcing it if we could write it as

filenames.map(&File->read)

or some nice looking special syntax.

Segway: I actually like this, despite it causing some PTSD (PHP Traumatic Stress Disorder.) Since the thin arrow is already used for lambdas, there's some neat intertextuality going on here. Much like foo&.bar is somewhat reminiscent of foo && foo.bar. 😀

@mikegee

This comment has been minimized.

Copy link
Contributor

mikegee commented Dec 27, 2016

I'm also a fan of &method, but admit the syntax is unwieldy. I like that you don't need to provide a name for the individual values being iterated over. It is a bit further along the procedural → declarative spectrum (if there is such a thing).

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Dec 29, 2016

Segway: I actually like this, despite it causing some PTSD (PHP Traumatic Stress Disorder.) Since the thin arrow is already used for lambdas, there's some neat intertextuality going on here. Much like foo&.bar is somewhat reminiscent of foo && foo.bar. 😀

Guess someone should file an upstream ticket suggesting this. I like it as well. :-)

@rrosenblum

This comment has been minimized.

Copy link
Contributor

rrosenblum commented Dec 31, 2016

A feature request has been created for this.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Jan 2, 2017

I meant someone should suggest this new syntax to Ruby's developers. :-) But anyways, having a cop for this would be useful as well I guess.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Jan 2, 2017

I'll close this ticket as I don't having issues with the code snippet that started this discussion.

@bbatsov bbatsov closed this Jan 2, 2017

@rrosenblum rrosenblum deleted the rrosenblum:performance_tweak branch Jan 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment