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

Variable assignment to self #1701

Open
cswoods6 opened this issue Feb 6, 2023 · 5 comments
Open

Variable assignment to self #1701

cswoods6 opened this issue Feb 6, 2023 · 5 comments

Comments

@cswoods6
Copy link

cswoods6 commented Feb 6, 2023

I was in the situation of replacing multiple method calls with variables and accidentally assigned the variable to itself without any modification. Would these be a useful smell to add?

Example:
foo = foo

@troessner
Copy link
Owner

I think it would be a nice addition - @mvz wdyt?

@cswoods6 if you want to come up with a PR we could certainly guide you towards the goal ;)

@mvz
Copy link
Collaborator

mvz commented Feb 8, 2023

Ah yes, I was still going to find out what that code does if this is the first time foo is assigned, and there's a method called foo as well.

It does look weird though so should definitely be caught by either Reek or RuboCop.

@JuanVqz
Copy link
Contributor

JuanVqz commented Oct 16, 2023

should it detect any local variable + method with the same name?
even foo = foo is going to be invalid?

@troessner
Copy link
Owner

So in my mind foo = foo should be a smell regardless if the right hand foo is the same variable or a method. If the right hand foo is a variable its obviously wrong and if its a method then its confusing, particularly if used without the parentheses. If its about saving additional method calls then the left-hand foo should be named differently or it might be smarter to memoize that function call so subsequent calls are "for free".

@mvz
Copy link
Collaborator

mvz commented Nov 16, 2023

I just tested this, and this assigns the method result to the variable:

foo = foo()

while this does not and leaves foo nil:

foo = foo

So, I agree the second form is bad and so does RuboCop.

I'm not sure this is really a code smell though? Maybe this should be left to RuboCop instead?

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